Skip to content

Comments

code fixes 1#31

Closed
Devesh21700Kumar wants to merge 10 commits intoreact-native-elements:masterfrom
Devesh21700Kumar:Dev
Closed

code fixes 1#31
Devesh21700Kumar wants to merge 10 commits intoreact-native-elements:masterfrom
Devesh21700Kumar:Dev

Conversation

@Devesh21700Kumar
Copy link

@Devesh21700Kumar Devesh21700Kumar commented Mar 10, 2021

1).restructured props in pages/home
2).relative imports from directories external to /src are not allowed in modern react versions so fixed them to adapt with latest versions
3).made Avatar a functional component

for issue #3

@netlify
Copy link

netlify bot commented Mar 10, 2021

Deploy preview for rne-playground ready!

Built with commit 75261b8

https://deploy-preview-31--rne-playground.netlify.app

@Devesh21700Kumar Devesh21700Kumar changed the title Dev code fixes 1 Mar 10, 2021
Copy link
Contributor

@tewarig tewarig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done... please mention the issue as well this pr is solving. I think it is #3

@Devesh21700Kumar
Copy link
Author

Devesh21700Kumar commented Mar 11, 2021

well done... please mention the issue as well this pr is solving. I think it is #3

thanks.. yeah i have mentioned it now
@pranshuchittora could u pls merge this too?

const Content = lazy(() => importMDX("./avatar.mdx"));
class App extends Component {
render() {
export default function App(){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you convert this to a named function. Default export of anonymous funcs is not recommended
https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-anonymous-default-export.md

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

export default HomePage;

const FeatureCard = (props) => {
const FeatureCard = ({banner,title,content}) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not required

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be fixed after TS migration of this repo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undo these changes as requested

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah @pranshuchittora reverted them. thanks

@@ -1,5 +1,5 @@
import React, { lazy, Suspense } from "react";
import { importMDX } from "mdx.macro";
import importMDX from "mdx.macro";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect, breaking the build.

react_devtools_backend.js:2430 ReferenceError: importMDX is not defined
    at index.jsx:7
    at 2.b56122d9.chunk.js:sourcemap:2
    at ys (2.b56122d9.chunk.js:sourcemap:2)
    at ul (2.b56122d9.chunk.js:sourcemap:2)
    at sl (2.b56122d9.chunk.js:sourcemap:2)
    at Zs (2.b56122d9.chunk.js:sourcemap:2)
    at 2.b56122d9.chunk.js:sourcemap:2
    at t.unstable_runWithPriority (2.b56122d9.chunk.js:sourcemap:2)
    at Ui (2.b56122d9.chunk.js:sourcemap:2)
    at Gi (2.b56122d9.chunk.js:sourcemap:2)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is importMDX defined for other files?

@pranshuchittora
Copy link
Member

Deploy preview for rne-playground ready!

Built with commit b16bc2d

https://deploy-preview-31--rne-playground.netlify.app

@Devesh21700Kumar pls check the build from your changes.

  1. Go to any component
  2. Click the Prop Explorer at the bottom right
  3. Check the console for the respective err

@Devesh21700Kumar
Copy link
Author

@pranshuchittora Hey, Thanks for the review.. I've fixed the Build errors and have made the export named instead of default.. please do check and merge if it is not giving any errors

@Devesh21700Kumar
Copy link
Author

Devesh21700Kumar commented Mar 13, 2021

@pranshuchittora I have reverted them now. Please check and merge. Thanks

@pranshuchittora
Copy link
Member

Pls resolve the conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants