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

Feature/theme switcher #525

Merged
merged 12 commits into from
Jan 13, 2020
Merged

Feature/theme switcher #525

merged 12 commits into from
Jan 13, 2020

Conversation

enikonemeth
Copy link
Contributor

@enikonemeth enikonemeth commented Dec 22, 2019

design

  • Remove repository change from the usermenu
  • Refactor to avoid/remove theme service

@enikonemeth enikonemeth added this to the Sprint 200 milestone Dec 22, 2019
@enikonemeth enikonemeth self-assigned this Dec 22, 2019
@sensenet
Copy link

sensenet bot commented Dec 22, 2019

Site name Url Last deploy
sn-react-component-docs-dev https://5e1c73a483ae738f51a44298--sn-react-component-docs-dev.netlify.com Mon Jan 13 2020 - 13:41:57 GMT+0000 (Coordinated Universal Time)
sn-app-dev https://5e1c73dc1a247a7f4d72a647--sn-app-dev.netlify.com Mon Jan 13 2020 - 13:42:54 GMT+0000 (Coordinated Universal Time)
sn-dms-demo-dev https://5e1c739613dc8f68326f3c40--sn-dms-demo-dev.netlify.com Mon Jan 13 2020 - 13:41:43 GMT+0000 (Coordinated Universal Time)

@codecov
Copy link

codecov bot commented Dec 22, 2019

Codecov Report

Merging #525 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #525   +/-   ##
========================================
  Coverage    91.36%   91.36%           
========================================
  Files          192      192           
  Lines         5209     5209           
  Branches      1302     1302           
========================================
  Hits          4759     4759           
  Misses         450      450
Impacted Files Coverage Δ
...ges/sn-controls-react/src/viewcontrols/NewView.tsx 96% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfef07c...e0ba9f9. Read the comment docs.

@herflis herflis removed this from the Sprint 200 milestone Jan 8, 2020
@zoltanbedi
Copy link
Contributor

There is strange behavior when I change theme the drawer items gets reinitialized and there are multiple getActions request sent. Please check if you can avoid that.

Copy link
Contributor

@zoltanbedi zoltanbedi left a comment

Choose a reason for hiding this comment

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

One more thing I would love to see in this pr is

const prefersDarkMode = useMediaQuery('(prefers-color-scheme: dark)')

this code somewhere.
https://web.dev/prefers-color-scheme/
It would be awesome if we would support this bydesign. 😊
@herflis Are you okay with this?

apps/sensenet/src/context/ThemeProvider.tsx Outdated Show resolved Hide resolved
@enikonemeth
Copy link
Contributor Author

There is strange behavior when I change theme the drawer items gets reinitialized and there are multiple getActions request sent. Please check if you can avoid that.

In use-drawer-item we are watching the settings (line 168-189) variable
const settings = useContext(ResponsivePersonalSetttings)
when we get the actions for drawer items.

export const ResponsivePersonalSetttings = React.createContext(defaultSettings.default)
...
ResponsivePersonalSetttings.Provider value={deepMerge(personalSettings.default, personalSettings.desktop)}>

In ResponsiveContextProvider the personalSettings is reactive so when it changes it will effect that we should update the drawer items as well.

To avoid this, we can remove the settings variable from the watches list in use-drawer-items, but it will cause lint warning...

@enikonemeth
Copy link
Contributor Author

One more thing I would love to see in this pr is

const prefersDarkMode = useMediaQuery('(prefers-color-scheme: dark)')

this code somewhere.
https://web.dev/prefers-color-scheme/
It would be awesome if we would support this bydesign. 😊
@herflis Are you okay with this?

In the original issue light theme was what I should use:

Another related issue is that maybe we could try out 'light' as the default theme, because not everyone was fascinated by the dark theme. 🕶

Copy link
Contributor

@zoltanbedi zoltanbedi left a comment

Choose a reason for hiding this comment

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

🌑

@enikonemeth enikonemeth merged commit daeb5ab into develop Jan 13, 2020
@enikonemeth enikonemeth deleted the feature/theme_switcher branch February 10, 2020 08:27
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