search

mapcomponents / react-map-components-maplibre

A react component framework for declarative GIS application development.
MIT License
116 stars 18 forks source link

Bug: Navigation Compass does not work in Dark Mode #132

Closed dBitech closed 1 year ago

dBitech commented 1 year ago

When attempting to use dark mode via theme switcher, the following error is thrown:

    at MlNavigationCompass.tsx:24:31
    at transformedStyleArg (createStyled.js:206:40)
    at handleInterpolation (emotion-serialize.browser.esm.js:139:24)
    at serializeStyles2 (emotion-serialize.browser.esm.js:253:15)
    at emotion-styled-base.browser.esm.js:123:24
    at emotion-element-c39617d8.browser.esm.js:38:12
    at renderWithHooks (react-dom.development.js:16305:18)
    at updateForwardRef (react-dom.development.js:19226:20)
    at beginWork (react-dom.development.js:21636:16)
    at HTMLUnknownElement.callCallback2 (react-dom.development.js:4164:14)

PR #131 attempts to resolve this

cioddi commented 1 year ago

Hi @dBitech thank you for the contributions.

I have taken a look at PR #131 and it looks like a useful feature/addition. Does this resolve the issue raised here or do you want us to look into the problem together. In the latter case please add the full error message and steps on how to reproduce it.

If you add your custom ThemeProvider below MapComponentsProvider, then this error should not be thrown. The theme prop of the custom theme should be provided as a function that has the current theme defined up in the React DOM as a parameter and returns the new theme:

<MapComponentsProvider>
  <ThemeProvider theme={outerTheme => ({...outerTheme, ...customTheme })}>
    <App />
  </ThemeProvider>
</MapComponentsProvider>

https://mui.com/system/styles/advanced/#theme-nesting

We need to be careful that the spread operator will not merge but replace object values. So in some case a more sophisticated merge-themes function might be required.

I will address the need for better/any documentation on theming in our next project meeting.

Edit: PR #131 is now also merged and will be in the next release. I'll keep this issue open for further discussion on the theming issue.

dBitech commented 1 year ago

Yes, this looks to have solved the immediate issue thank you. I'm rather new to react, the theme switching was based on code copied out of the powerplant App from react-map-components-apps since I could not find any other meaningful documentation or examples.

cioddi commented 1 year ago

I just had to fix a similar issue in one of our apps.

https://github.com/mapcomponents/react-map-components-apps/commit/205d91a934d0ef60f1435ee87561b0ab4a822df9#diff-2cd3fb460e40cd1ae7dc4f21eec43ab7834a79805026cdd8daf449eb6c1fd848R5

To make sure the higher level props don't overwrite each other @mui/utils exports the function deepmerge. In your case you could use it as follows:

import { deepmerge } from '@mui/utils';

<MapComponentsProvider>
  <ThemeProvider theme={outerTheme => deepmerge(outerTheme, customTheme )}>
    <App />
  </ThemeProvider>
</MapComponentsProvider>
cioddi commented 1 year ago

Just released v0.1.86 that includes your changes.