Skip to content

Add react tip button#32

Closed
shapkarin wants to merge 3 commits intomasterfrom
feature/button-react
Closed

Add react tip button#32
shapkarin wants to merge 3 commits intomasterfrom
feature/button-react

Conversation

@shapkarin
Copy link
Copy Markdown
Contributor

@shapkarin shapkarin commented Oct 22, 2020

closes #21

@shapkarin shapkarin requested review from davidyuk and mradkov October 22, 2020 12:52
Copy link
Copy Markdown
Member

@davidyuk davidyuk left a comment

Choose a reason for hiding this comment

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

I don't think that all environments will support es6 imports, classes, and arrow functions, so proposing to pass button-react through webpack.

@shapkarin shapkarin force-pushed the feature/button-react branch from dee983f to 08db3a4 Compare October 26, 2020 05:58
@davidyuk
Copy link
Copy Markdown
Member

@shapkarin Any response to the first comment about es6 features and webpack?

@shapkarin
Copy link
Copy Markdown
Contributor Author

@shapkarin Any response to the first comment about es6 features and webpack?

@davidyuk I will add

Copy link
Copy Markdown
Member

@davidyuk davidyuk left a comment

Choose a reason for hiding this comment

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

Document this feature in the README, please.

@shapkarin
Copy link
Copy Markdown
Contributor Author

Document this feature in the README, please.

Added basic version.

@shapkarin shapkarin force-pushed the feature/button-react branch 2 times, most recently from f04111c to ff038ad Compare November 11, 2020 13:18
@shapkarin shapkarin requested a review from davidyuk November 11, 2020 13:18
Comment on lines +30 to +31
"prop-types": "^15.7.2",
"react": "^17.0.0"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Currently, these dependencies are inlined into the bundle, let's use externals to keep them extracted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I hope that will help with that, I will add it

  externals: {
    'prop-types': 'prop-types',
    'react': 'react'
  },

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The general idea is not to hope but to test it using a bundle analyzer and trying to use it in different environments

Copy link
Copy Markdown
Contributor Author

@shapkarin shapkarin Nov 21, 2020

Choose a reason for hiding this comment

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

I will check it at Monday's morning

@shapkarin shapkarin requested a review from davidyuk November 16, 2020 13:52
@shapkarin shapkarin force-pushed the feature/button-react branch from 7f2c602 to b94c723 Compare November 17, 2020 00:34
@shapkarin
Copy link
Copy Markdown
Contributor Author

@davidyuk squashed into one commit, please check this PR

Copy link
Copy Markdown
Member

@davidyuk davidyuk left a comment

Choose a reason for hiding this comment

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

Not all issues from the last review are resolved

Copy link
Copy Markdown
Member

@davidyuk davidyuk left a comment

Choose a reason for hiding this comment

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

In general, this PR looks fine, but there is one thing related to the documentation that I would like to check before merging this. I will come back later to this PR.

@shapkarin
Copy link
Copy Markdown
Contributor Author

shapkarin commented Nov 25, 2020

@davidyuk fixed and I clean build folder and run it once again, looks that all is fine. I will change commits history today.

Yury Shapkarin and others added 2 commits November 25, 2020 17:38
Update webpack.config.js

Co-authored-by: Denis Davidyuk <denis_davidyuk@hotmail.com>

again temporary commit just to track

fix README.md anchor links

fix genConfig option name
Co-authored-by: Denis Davidyuk <denis_davidyuk@hotmail.com>
Copy link
Copy Markdown
Member

@davidyuk davidyuk left a comment

Choose a reason for hiding this comment

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

In #38 I have implemented a more general approach with additional examples allowing to test that. Proposing to close this and merge that instead.

super(props);
this.button = React.createRef();
this.componentDidMount = this.componentDidUpdate = () =>
createButton(this.button.current, this.props);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While working on #38 I have found that the replacement of the DOM node breaks subsequent component updates. I have fixed it there by using a different API.

target: 'web',
externals: {
'prop-types': 'prop-types',
'react': 'react'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, this is not working if imported using the script tag because React exposes itself there as React, it is fixed in #38

@shapkarin shapkarin closed this Dec 2, 2020
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.

Add react version @aeternity/superhero-button/react

2 participants