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

OCLOMRS-1065: Upgrade the MUI to version 5 in dictionary manager #760

Merged
merged 15 commits into from
Dec 9, 2021

Conversation

suruchee
Copy link
Contributor

JIRA TICKET NAME:

Upgrade the mui to version 5 in dictionary manager

Summary:

Upgrade to MaterialUI 5

@suruchee suruchee requested a review from ibacher October 26, 2021 09:27
@suruchee suruchee changed the title Oclomrs 1065 OCLOMRS-1065:Upgrade the mui to version 5 in dictionary manager Oct 26, 2021
@suruchee
Copy link
Contributor Author

suruchee commented Oct 26, 2021

Hi @ibacher, I have tried to upgrade to MUI 5 but the default theme provider is not supported in MUI5 so I got an error at. So, I added a ThemeProvider in root file but still I got the same error. So I tried using Theme from "@mui/material" which do not give the same UI as expected. Please have a look at the issue I am getting. I still need to do more Q/A in this PR but I am stuck at theme.

@suruchee
Copy link
Contributor Author

Hi @ibacher, @hadijahkyampeire helped me figure out the issue with the navbar, it was not working because mui5 has added px as the default unit. But still, there are some UI improvements needed so just changing this PR to draft.

@suruchee suruchee marked this pull request as draft October 28, 2021 15:52
@suruchee suruchee marked this pull request as ready for review November 11, 2021 07:01
@coveralls
Copy link

coveralls commented Dec 3, 2021

Coverage Status

Coverage decreased (-0.004%) to 46.402% when pulling fb4c8e4 on suruchee:OCLOMRS-1065 into f1e705d on openmrs:master.

Comment on lines +15 to +16
"@emotion/react": "^11.4.1",
"@emotion/styled": "^11.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need "@emotion/react" and "@emtion/styled"? I know that MUI is using them under the hood, but we don't seem to be actually using them anywhere and it's better to keep the dep list as minimal as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you so much @ibacher, for the review. I had not used emotion at the beginning but it ended up with some error Unhandled Rejection error in Reducer. And was fixed by adding emotion so had to add this dep.

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Aside from the minor comment I've left, I'm happy for this to be merged in. Nice work @suruchee!

@ibacher
Copy link
Member

ibacher commented Dec 8, 2021

Where we have to use test ids, can we try using findByLabelText(). The new version of MUI moves the data from the title attribute to the aria-label.

@ibacher ibacher changed the title OCLOMRS-1065:Upgrade the mui to version 5 in dictionary manager OCLOMRS-1065: Upgrade the MUI to version 5 in dictionary manager Dec 9, 2021
@ibacher ibacher merged commit 703b174 into openmrs:master Dec 9, 2021
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