Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to material-ui v4 #46

Merged
merged 10 commits into from
Nov 13, 2019
Merged

Migrate to material-ui v4 #46

merged 10 commits into from
Nov 13, 2019

Conversation

helfi92
Copy link
Owner

@helfi92 helfi92 commented Oct 22, 2019

Closes #36.

@helfi92
Copy link
Owner Author

helfi92 commented Oct 28, 2019

Do you know why this style broke in latest material-ui? It’d be good to know why.

@edil-it-them
Copy link
Collaborator

I added marginRight: 0 to expandIcon that overrides .MuiIconButton-edgeEnd css rule with marginRight: -12px.
My commit made so many changes in yarn.lock. Is it ok?

@helfi92
Copy link
Owner Author

helfi92 commented Oct 28, 2019

I added marginRight: 0 to expandIcon that overrides .MuiIconButton-edgeEnd css rule with marginRight: -12px.

What I'm trying to understand is why it was working in previous material-ui but broke in material v4.

My commit made so many changes in yarn.lock. Is it ok?

Yeah, that's okay.

@edil-it-them
Copy link
Collaborator

Do you know why this style broke in latest material-ui? It’d be good to know why.

I'm trying to find it in material-ui changelog and documentation. Don't understand why expandIcon gets edgeEnd now. It's too late in my timezone, I'll try to find it tomorrow

@helfi92
Copy link
Owner Author

helfi92 commented Oct 29, 2019

Sounds good.

@edil-it-them
Copy link
Collaborator

edil-it-them commented Oct 29, 2019

This PR added edge props to IconButton in material-ui v4, default value is false. But expandIcon in ExpansionPanelSummary adds edge: 'end' to Icon as default. May be it's material-ui bug )
We can add IconButtonProps={{edge:"false"}} prop to ExpandPanelSummary and we can remove extra classes

@helfi92
Copy link
Owner Author

helfi92 commented Oct 29, 2019

We can add IconButtonProps={{edge:"false"}} prop to ExpandPanelSummary and we can remove extra classes

I like this solution. Let's do it :)

Copy link
Owner Author

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

Code looks good but I just found two other regressions when comparing to https://hassanali.me/material-ui-treeview/ :(

  1. Spacing between edge and the button

Before

Screen Shot 2019-10-29 at 1 16 54 PM

After

Screen Shot 2019-10-29 at 1 17 15 PM

  1. Items too close to each other

Before

Screen Shot 2019-10-29 at 1 17 52 PM

After

Screen Shot 2019-10-29 at 1 17 48 PM

@edil-it-them
Copy link
Collaborator

I'm not sure about padding between items, but it must work after overrides in theme.js.
I can't run yarn start:styleguide, cause this warning
theme.js not working in http://localhost:5000/ development server(

warning

@helfi92
Copy link
Owner Author

helfi92 commented Oct 29, 2019

Does yarn run styleguidist server work?

@helfi92
Copy link
Owner Author

helfi92 commented Oct 29, 2019

Another thing you can try is rm -rf node_modules/ && yarn && yarn start:styleguide.

@edil-it-them
Copy link
Collaborator

Another thing you can try is rm -rf node_modules/ && yarn && yarn start:styleguide
Does yarn run styleguidist server work?

I've tried both, not working. Catch the same error

src/components/MuiTreeView/index.jsx Show resolved Hide resolved
src/theme.js Outdated Show resolved Hide resolved
@helfi92
Copy link
Owner Author

helfi92 commented Oct 30, 2019

I've tried both, not working. Catch the same error

Ah, it's possible that you introduced code that doesn't compile. Check git status for any code introduced by accident like an extra comma or something.

@edil-it-them
Copy link
Collaborator

edil-it-them commented Oct 31, 2019

import StyleGuide from 'react-styleguidist/lib/client/rsg-components/StyleGuide/StyleGuideRenderer'; Works for me

There are two more regressions
Before

before

After

after

It seems like after migration css rules work different

css after

In old version .makeStyles-panelExpanded override .MuiExpansionPanel-root.Mui-expanded
Should we change it?

@helfi92
Copy link
Owner Author

helfi92 commented Oct 31, 2019

In old version .makeStyles-panelExpanded override .MuiExpansionPanel-root.Mui-expanded
Should we change it?

Yes please :)

@edil-it-them
Copy link
Collaborator

I followed this example to make it work correctly

Copy link
Owner Author

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

Lovely! The only thing missing as far as i can tell is to fix the code text color:

Screen Shot 2019-11-01 at 8 27 25 AM

The property keys are hard to see.

@edil-it-them
Copy link
Collaborator

Is it OK now?

@helfi92
Copy link
Owner Author

helfi92 commented Nov 6, 2019

Yeah, looks good :) Do you know why this happened by any chance?

@edil-it-them
Copy link
Collaborator

edil-it-them commented Nov 6, 2019

React styleguide now uses Prism for codeviewer and these values are default. So I changed some of these values in theme.js
In demo styleguide uses CodeMirror for codeviewer

Major changes:
* Migrate to material-ui v4
* Rewrite component with react hooks
@helfi92 helfi92 changed the title [WIP] Migrate to material-ui v4 Migrate to material-ui v4 Nov 13, 2019
@helfi92
Copy link
Owner Author

helfi92 commented Nov 13, 2019

Awesome work @Rolikasi. Thank you for the help!

@helfi92 helfi92 merged commit 8e0e52e into master Nov 13, 2019
@helfi92 helfi92 deleted the update-material-ui branch November 13, 2019 16:50
@helfi92
Copy link
Owner Author

helfi92 commented Nov 13, 2019

Released in material-ui-treeview@4.0.0 🎉

@edil-it-them
Copy link
Collaborator

Woohoo!!!

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.

Material-UI upgrade?
2 participants