Chore: Migrate Material UI Dependencies to version 4.X.X #90
Conversation
priscilawebdev
changed the title
There was a problem hiding this comment.
The linting rules changes are great, they makes code readability better. It could be great if you can extract in other PR, but if you cannot, doesn’t matter, good job.
In the other hand, you should take care on tests and what snapshots generate, because I cannot see what Author render as a snapshot, and that’s a bad snapshot testing pattern.
Some changes and it will be 💯
| @@ -27,8 +27,7 @@ describe('<Author /> component', () => { | |||
| })); | |||
|
|
|||
| const Author = require('./Author').default; | |||
| const wrapper = shallow(<Author />); | |||
| expect(wrapper.html()).toMatchSnapshot(); | |||
| expect(<Author />).toMatchSnapshot(); | |||
There was a problem hiding this comment.
It's necessary to shallow render the component. Take a look at the snapshot generated, it's only an Author tag and not rendered, so it's a wrong test
There was a problem hiding this comment.
@sergiohgz that is true...reason Snapshots are still good to have..thank you :-) Well I am getting an error when I shallow this component. it doesn't accept react hooks...do you have some idea of how to fix this issue? thanks 👍 :-)
There was a problem hiding this comment.
I’ve just read that Enzyme has support for testing hooks (but don’t complete and I don’t know if they are released as stable) and some patterns like wrapping hooks in a component to test how they work. Do that help you? I don’t have a computer near to investigate more 😢
Codecov Report
@@ Coverage Diff @@
## 4.x-master #90 +/- ##
==============================================
- Coverage 66.07% 63.85% -2.22%
==============================================
Files 86 89 +3
Lines 849 855 +6
Branches 140 139 -1
==============================================
- Hits 561 546 -15
- Misses 279 300 +21
Partials 9 9
Continue to review full report at Codecov.
|
priscilawebdev
changed the title
| @@ -1,5 +1,5 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|
|||
| exports[`<Author /> component should render the component in default state 1`] = `"<ul class=\\"MuiList-root-1 MuiList-padding-2 MuiList-subheader-4\\"><h3 class=\\"MuiTypography-root-5 MuiTypography-subheading-12 css-hyrz44 e1xuehjw0\\">Author</h3><li class=\\"MuiListItem-root-41 MuiListItem-default-44 MuiListItem-gutters-49 css-z8a2h0 e1xuehjw1\\"><a href=\\"mailto:verdaccio.user@verdaccio.org?subject=verdaccio@4.0.0\\" target=\\"_top\\"><div class=\\"MuiAvatar-root-53\\"><img alt=\\"verdaccio user\\" src=\\"https://www.gravatar.com/avatar/000000\\" class=\\"MuiAvatar-img-55\\"/></div></a><div class=\\"MuiListItemText-root-56\\"><span class=\\"MuiTypography-root-5 MuiTypography-subheading-12 MuiListItemText-primary-59\\">verdaccio user</span></div></li></ul>"`; | |||
| exports[`<Author /> component should render the component in default state 1`] = `<Authors />`; | |||
There was a problem hiding this comment.
🚨 You are removing snapshots. Any reason ??
There was a problem hiding this comment.
Please seen my answer to @sergiohgz above :-)
|
We should stick with the main topic of the PR, migrate the library. I haven’t look yet the migration guide but (as I already discuss with Pris) eslint and other things must be addressed in a different PR. We need to review why the coverage decreased 2% and please we should not modify any unit test if is not required or topic related (unless is documented the reason). Let’s try keep things small so is easy to review. 🙏 (on my mobile now) I will take a look the changes later on. |
|
Thank you for your feedback! 👍 I already removed the changes I did in the eslint file (now tests are broken), but as I said previously, I was going to open new PR's for the changes. Regarding the test I could not make shallow or mount from enzyme play well with react hooks...Please see: reactjs/rfcs#71.. I appreciate if you have some solution... ps: As I suggested before .. I would rather use react-testing-library..It fits better our needs..and plays well with hooks https://github.com/testing-library/react-testing-library
|
|
I'll hold this until until we fix all bugs, currently the master is blocked since we have visual bugs and I could not add the latest version to the |
…:verdaccio/ui into chore/87_update_material_ui_dependencies
|
I would rather wait for this enzymejs/enzyme#2011
Enzyme will support hooks soon or later, I'd prefer put the effort in their shoulders rather in ours. So, let's wait for it 🍿 . |
|
@juanpicado ok so let's wait 😉 |
Type: Chore
The following has been addressed in the PR: closes #85
There is a related issue?
No
Unit or Functional tests are included in the PR
No..it was not necessary
Description:
As part of our process, we try to keep our libraries up to date. In this PR the updated libraries were:
As the version 4 of Material UI has breaking changes, we followed this guideline: https://material-ui.com/guides/migration-v3/
As we are still not using theme, the breaking changes didn't affect us much, with the exception of forwardRef. It was necessary to include ForwardRef in the following components:
Why did I move some components to the "primitive" folder:
I found necessary to move the above components to the "primitive" folder, because I was getting confused. Some of the components are Material UI - Only and some are verdaccio's patterns.
I also think that once we have a theme, we need to limit some of the passed props, so that's one more reason to keep the primitive material components in a separate folder.
Why did I add the rule "react/display-name":
Well, I think there is a problem (I will investigate more), but it is not necessary to always add a component name, if it has already one.