Upgrade to MUI 5

[jacobwod]

The why: https://mui.com/blog/mui-core-v5/
The how: https://mui.com/guides/migration-v4/
The who: (funny, eh?) @Hallbergs and @jacobwod will give it a go. However, we won't be able to test all plugins as some code isn't easily setup in our environments. This is mainly a concern for Västtrafik (@sweco-semtto) and Varberg (FME, KID/FIR - @maan002, @jesade-vbg).

Rough roadmap:

• new branch from develop
• follow the guide step-by-step, small commits and regular testing
• merge changes into develop
• merge to master
• remove @mui/styles from package.json. This step is a breaking change.

The progress can be found in this branch if someone wants to test something before we merge it to master.

Progress:

• Migrate all core components.
  • Announcement
  • Drawer
  • FeatureInfo
  • Search
  • SrShortcuts
  • Alert.js
  • App.js - Special styling for stackbars on small screens left, see Snackbars covers window controls (On mobile) #731.
  • Card.js
  • CookiNotice.js
  • Diagram.js
  • Dialog.js
  • HajkThemeProvider.js
  • Introduction.js
  • PanelHeader.js
  • PluginControlButton.js
  • PluginWindows.js
  • PopPanel.js
  • Table.js
  • Window.js
• Migrate all controls.
  • Attribution.js
  • Information.js
  • MapCleaner.js
  • MapResetter.js
  • MapSwitcher.js
  • PresetLinks.js
  • Rotate.js
  • ScaleLine.js
  • ThemeToggler.js
  • Zoom.js
• Migrate all plugins.
  • Anchor
  • Bookmarks
  • Buffer
  • Collector - Would love if someone with this plugin set-up could run some ui-tests.
  • Coordinates
  • DocumentHandler
  • Draw
  • Dummy
  • Edit
  • Export
  • Fme
  • Informative Ongoing discussion, deprecated? Yes, we're not migrating this. Another change: migrated!
  • LayerSwitcher
  • Location
  • Measure
  • Print
  • Routing
  • StreetView
  • Suggest
  • TimeSLider
  • VTSearch - Will be handled separately in VT's Hajk-fork.
  • BaseWindowPlugin.js
• Merge to develop
• Merge to master
• Remove @mui/styles from package.json. This will make the old styling unusable.

[sweco-semtto]

Okay from Västtrafik's point of view.

[jacobwod]

Here's a list of what's left to be done after we've completed the main migration:

• @sweco-semtto : We haven't migrated @material-ui/pickers, see this page for instructions. https://mui.com/guides/pickers-migration/
• @jacobwod @Hallbergs Ensure that print from DocumentHandler looks as it should (DocumentHandlerTheme.js)

[sweco-semtto]

Thanks! When Hajk is to be upgraded next time, we will also migrate @material-ui/pickers.

[Hallbergs]

I've found that we're getting an error when we're using a custom theme on the documentHandler. E.g. the following documentHandlerTheme.json throws an error:
{ "typography": { "fontSize": 14, "h2": { "fontSize": "1.5rem", "fontWeight": 800, "margin-bottom": "0.25em" }, "h3": { "fontSize": "1.25rem", "fontWeight": 800, "padding-top": "1em", "margin-bottom": "0.25em" }, "h4": { "fontSize": "1.0rem", "fontWeight": 800, "padding-top": "1em", "margin-bottom": "0.25em" }, "h5": { "fontSize": "0.88rem", "fontWeight": 800, "padding-top": "1em", "margin-bottom": "0.25em" }, "h6": { "fontSize": "0.88rem", "fontWeight": 800, "padding-top": "1em", "margin-bottom": "0.25em" }, "body1": { "fontSize": "0.88rem", "margin-bottom": "0.625em" }, "subtitle2": { "fontSize": "0.88rem", "fontStyle": "italic" } } }

The error we're seeing is the following:
Using kebab-case for css properties in objects is not supported. Did you mean marginBottom?

Seems like the theme is a bit mixed between camelCase and kebab-case, fontSize is camelCase in the example above, and padding-top is kebab-cased. I've got a feeling that this error has been around even before the mui5 migration, but supressed when not in dev mode.

Fortunately this is an easy fix, just make sure to only use camelCase in documentHandlerTheme.json.

[Hallbergs]

I've given it a go to migrate VTSearch as well. Approx 50% done, progress can be found in the following branch:
https://github.com/hajkmap/Hajk/tree/feature/871-vtsearch-mui-v5

There are some components that i can't test, so i would be happy if someone from vt can give it a quick look sometime :)

[sweco-semtto]

Yes, marginBottom is correct. We should have used camelCase all the way.

[Hallbergs]

I've updated documentHandlerTheme.json in the MUI-migration-branch, so it'll be fixed when we merge it :) I've also almost migrated VTSearch completely, there is one component to go, and then some testing of course.

[sweco-semtto]

Many thanks, it's a big step forward. I will report back to Västtrafik that migration will not be such a big effort for us.

[Hallbergs]

Quick update: The migration is about to be completed. Only the Documenthandler- and VTSearch-plugin (almost done in another branch) to go.

[jacobwod]

Fantastic work as always @Hallbergs!

[Hallbergs]

One difference after the migration will be that the background-color in dark-mode is a bit darker than before. This is done by MUI to improve the contrasts. See images below:

Before:

After:

In my opinion it looks better than before! Any other opinions?

[jacobwod]

There's definitively a reason for this change, such as the increased contrast that we've talked about. I like it. 👍

[jacobwod]

Recent merge of develop into this feature branch has solved one older issue (#917).

We have a couple of minor fixes left however. One noteworthy is the behavior inside LayerSwitcher: the size of lables depends on the visibility of FeatureInfo window:

Skarminspelning.2022-01-17.kl.13.16.02.mov

[Hallbergs]

Oh some css-specificity-errors! How fun!!

[jacobwod]

Yeah, it's probably some more generic part then just this override in FeatureInfo. I can see funny things going on as soon as any window with custom styling gets "focus" (or is active/in front/injects its' custom styling). See this one:

Skarminspelning.2022-01-17.kl.13.25.02.mov

I'll investigate!

[Hallbergs]

I think we'll have to dig into the migration-guide 😵‍💫

[jacobwod]

Alright, the two issues mentioned and illustrated with videos closed as of af55efe!

[Hallbergs]

Fantastic work! So all the issues came from bad imports?

[jacobwod]

Yeah, should've migrated all related components before panicking 😉

[jacobwod]

@jesade-vbg, you've previously added a custom styling to oursnackbars.

Any idea how it is supposed to look? It looks like this on small viewports currently. What should be different?

[jesade-vbg]

Yes, the snackbar was raised so "visa koordinat" was fully visible. The snackbar also made "visa koordinat" unclickable so something fixed that (pointerEvents:none). Same thing if the snackbar was on top, but in this case the snackbar was lowered below searchbox. These changes was (is?) really needed.

I just took a quick look, and yes, it does not work any more. I guess bacause of structural changes a while back. Both positioning and pointerEvents: none are defective. Should I get it to work in the current develop branch?

Note: This might cause some headache:

Desired look (and pointer-events should work on the "Visa koordinat"-bar, marked with green):

[jacobwod]

Thanks. One more question: is it supposed to be raised when the plugin window is minimised, only or should the snackbar be raised all the time? Currently it isn't raised in the window maximised-mode, and it seems to work out fine:

So I'm thinking that we should either: a) raise the snackbar when window minimised, or b) raise the minimised window itself so it appears above the snackbar. Opinions?

[jacobwod]

Hi @jesade-vbg, I took a look at how it was previously, e.g. in Hajk 3.7 (which is available on karta.halmstad.se). The override doesn't seem to work. I think that's something very specific in your Varberg setup that you target, but it never worked for anyone else:

[jesade-vbg]

It worked fine in the beginning of April. When it stopped working I dont know.
I just tested the commit eef7fa2, and it works.
The first commit related to the issue is dated Mar 31, and the last 1 April.

[jacobwod]

Well, I agree that the placement of snackbars on small screens is indeed an issue, but it's a really small one. Especially compared to the benefits this updated gives us, such as a way smaller bundle size and latest packages across the board (=security). In addition, the fix hasn't worked for a couple of the latest releases, and no-one cared to fix it then.

To sum up: it is an issue, but I don't see it as a deal-breaker. The issue #731 should be re-opened and handled separately from #871. (If someone who really cares wants to fix it as part of #871, the contribution is welcomed, of course.)