feat: replace withStyles wrapper with useEffect and remove customStyles property #873
+696
−1,230
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Deploy preview for fundamental-react ready! Built with commit 25ae8ff |
jbadan
commented
Feb 17, 2020
| @@ -23,6 +22,12 @@ class DatePicker extends Component { | |||
| this.calendarRef = React.createRef(); | |||
| } | |||
|
|
|||
| componentDidMount() { | |||
| if (!this.props.disableStyles) { | |||
| require('fundamental-styles/dist/popover.css'); | |||
There was a problem hiding this comment.
This whole section can be removed if #858 is merged first. That rebuilds this with a popover so we get those styles from popover component.
jbadan
changed the title
skvale
reviewed
Feb 17, 2020
| @@ -498,6 +503,4 @@ Shellbar.propDescriptions = { | |||
| subtitle: 'Displays an application context. Should be used rarely.' | |||
| }; | |||
|
|
|||
| export { Shellbar as __Shellbar }; | |||
|
|
|||
| export default withStyles(Shellbar, { cssFile: ['shellbar', 'product-switch'], fonts: true }); | |||
There was a problem hiding this comment.
should this file also require('fundamental-styles/dist/fonts.css'); since fonts was true?
There was a problem hiding this comment.
Nope - there's a bunch of other components imported into this component that already require fonts. That was an oversight to include them in the first place with withStyles
skvale
reviewed
Feb 17, 2020
| .add('custom styles', () => ( | ||
| <Image | ||
| customStyles={require('../../utils/WithStyles/customStylesTest.css')} | ||
| photo='https://content.fortune.com/wp-content/uploads/2019/07/hippocorn.jpg' |
There was a problem hiding this comment.
just so we know, this is what we're losing :(
skvale
reviewed
Feb 17, 2020
greg-a-smith
approved these changes
Feb 17, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
By removing dynamic requires, consumers will not be bringing in all of fundamental-styles with each component.
"...This means dynamic requires are supported but will cause all matching modules to be included in the bundle." https://webpack.js.org/guides/dependency-management/#require-with-expression
I decided to remove
withStylesinstead of repurposing it. Two reasons:cssFile: require('fundamental-styles/dist/button.css)towithStyleswill cause the css to always be loaded and not respectdisableStylesprop.cssFile: 'fundamental-styles/dist/button.css'and then calling it likerequire(cssFile)inwithStylesdoes not work because of the following:"Fully dynamic statements, such as import(foo), will fail because webpack requires at least some file location information.The import() must contain at least some information about where the module is located, so bundling can be limited to a specific directory or set of files.
Just in case someone wonders, the same rule applies for dynamic require statements"
https://stackoverflow.com/questions/44166393/uncaught-error-cannot-find-module-when-using-dynamic-import-for-javascript
On the remaining class components (which we need to convert at some point), I used
componentDidMountand on functional componentsuseEffect.I also removed
customStylesprop. Because we are not using a hoc anymore, we can just import our custom styles in our wrapper inside nui-widgets.