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 Doc-It to MUI5 and latest dependencies #601

Merged
merged 50 commits into from
Dec 21, 2022
Merged

Conversation

joebochill
Copy link
Collaborator

@joebochill joebochill commented Nov 23, 2022

Fixes #519 and maybe others.

Changes proposed in this Pull Request:

  • Rescaffold the application
    • Using MUI5, Craco, MDX, React Router v6
    • Convert all styles to SX
  • Clean up issues where I could see them
  • Function test
  • Logged a few bugs for new items I encountered
  • Added a theme picker debug mode (triple click on the SharedToolbar to trigger the picker) to test themes quickly without having to mess with the scheduler (this implementation is rough and could be cleaned up)
  • Had to disable prettier for MDX files because of some conflicts with how prettier works with the newer MDX comment syntax (upstream issue)

Notable areas that are iffy:

  • The new theme structure causes some issues with our holiday themes...I only fixed them for Thanksgiving since that was the one that was live...we will need a new issue to fix the others.
  • With 2000+ icons on the icon browser page (just from updating the package, I did NOT update the metadata file), it's starting to get a little laggy...I logged an issue to set up virtualization for this list to try to mitigate that.
  • Currently using alpha version of themes and components until we have official releases

Screenshots:

image

image

image

To Test:

  • Clone the repo
  • yarn start
  • go nuts

@github-actions
Copy link

github-actions bot commented Nov 23, 2022

Visit the preview URL for this PR (updated for commit f40e73c):

https://blui-doc-it--pr601-feature-mui5-migrati-j6cixaa3.web.app

(expires Fri, 23 Dec 2022 16:54:11 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 590ed6fb71e127776beff0a05819e22fd4307b8f

@joebochill
Copy link
Collaborator Author

Not sure what the deal is with the link checker (I'm about ready to be done with it in the CI steps)...all of the ones being flagged are working.

@jeffvg
Copy link
Contributor

jeffvg commented Nov 28, 2022

just looking around in here. I see this warning

  • The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type"

image

Didn't we see this somewhere else before?

@joebochill
Copy link
Collaborator Author

just looking around in here. I see this warning

  • The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type"

Didn't we see this somewhere else before?

Yeah, it's an emotion thing. There are a couple places where we use :first-child (some of them could probably be replaced, others might actually have to be :first-child because we don't know what the type is). In either event, we're not using server-side rendering, so it's not a concern, and looking at the deployed dev-docs (which have the same rules in the markdown.css), the warning does not appear in the production build.

@jeffvg
Copy link
Contributor

jeffvg commented Nov 28, 2022

should sx borderColor be passed in here?

image

without renders like this
image

with borderColor renders like this
image

@joebochill
Copy link
Collaborator Author

should sx borderColor be passed in here?

without renders like this

with borderColor renders like this

This is actually a bug in our theme (https://github.com/brightlayer-ui/react-themes/blob/dev/src/blueTheme.ts#L185). We are setting the border color to 'divider' for all outlined buttons...we should have another themeOverride that tells outlinedInherit to use 'currentColor'.

Passing in white to the sx will make it look correct for the blue theme, but any other holiday theme (or dark theme) that needs a different color border will be broken. It should ultimately get fixed in the theme (if you want to log a bug to track it that would be great)

@jeffvg
Copy link
Contributor

jeffvg commented Nov 28, 2022

should sx borderColor be passed in here?
without renders like this
with borderColor renders like this

This is actually a bug in our theme (https://github.com/brightlayer-ui/react-themes/blob/dev/src/blueTheme.ts#L185). We are setting the border color to 'divider' for all outlined buttons...we should have another themeOverride that tells outlinedInherit to use 'currentColor'.

Passing in white to the sx will make it look correct for the blue theme, but any other holiday theme (or dark theme) that needs a different color border will be broken. It should ultimately get fixed in the theme (if you want to log a bug to track it that would be great)

https://github.com/brightlayer-ui/react-themes/issues/50

@jeffvg
Copy link
Contributor

jeffvg commented Nov 28, 2022

visually it looks good with the exception of the extra spacing (marginTop mt) on the main landing page and other category landing pages. Looks like everything was pushed down like 24px or so

before
image

after
image

@jeffvg
Copy link
Contributor

jeffvg commented Nov 28, 2022

last thing I noticed is the TOC navigation getting chopped off around 1200px wide when manually resized but looks ok on fixed settings (laptop, mobile)
image

Copy link
Contributor

@huayunh huayunh left a comment

Choose a reason for hiding this comment

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

In addition to what Jeff said...

1

That console log triggers my nerve

2

Table of content differs in border color and font size. (Left: existing; right: new)

image

3

Convert your TODO.txt at the root to issue reports

4

On the Typography page, the table color should be the same as its background
image

5

Are you planning to get rid of all the useTheme hooks? Or some of the display: 'flex' styles using MUI's <Stack> component? Or migrate to MUI Grid v2? Should we do the update in the future, or is it "it ain't broke, don't fix it"?

6

Just curious, are you using MUI's "codemods" for the migration?

7

Just want to say thanks for the hard work during the holiday

@joebochill
Copy link
Collaborator Author

I would recommend bundling up any lingering style issues into a new story. This was really just to get the MUI 5 upgrade to a workable state. There's still plenty of opportunities for further enhancement (switching out divs for box, replacing flex styles with Stacks, etc.) — I tried to get what was in front of me, but didn't get everything and yeah if it wasn't broke I didn't fix it.

If anybody wants to tackle any of the remaining issues, feel free to jump on this PR (or we can close it and leave the MUI5 branch as a working branch until we have everything we want cleaned up). I won't have the luxury of vacation time to work on it for another month :P

Regarding the useTheme hooks...I got rid of them where I could. There are some places where it was still necessary because a certain style cannot be achieved in sx (like breakpoints.down media queries or if we need to apply an alpha value to a theme color).

I didn't use the codemod, I just scaffolded a new CLI project, added in all of the config for craco, mdx, etc. by hand and then moved over the src files and cleaned up things that were broken manually. Codemods may have saved some time, but 🤷‍♂️

Copy link
Contributor

@bkarambe bkarambe left a comment

Choose a reason for hiding this comment

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

…t-mui-v5-migration

Tidy up docit mui v5 migration
@joebochill
Copy link
Collaborator Author

@joebochill Shall we cover #609 in this PR?

No, that's not directly related to MUI 5, so we will handle it separately.

@huayunh huayunh self-assigned this Dec 21, 2022
Copy link
Contributor

@huayunh huayunh left a comment

Choose a reason for hiding this comment

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

Presto!

@huayunh huayunh merged commit 882d1d5 into dev Dec 21, 2022
@delete-merged-branch delete-merged-branch bot deleted the feature/mui5-migration branch December 21, 2022 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Update react-router to latest version 6.2.1
6 participants