-
Notifications
You must be signed in to change notification settings - Fork 156
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
Introduce themeswitcher #6240
Introduce themeswitcher #6240
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
d3e2928
to
c30d211
Compare
@@ -17,6 +17,7 @@ export const loadTheme = async (location = '') => { | |||
return defaults | |||
} | |||
const theme = await response.json() | |||
console.log('theme', theme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dev leftover (well... still WIP... but shouldn't be committed)
@@ -109,7 +118,7 @@ const getters = { | |||
return parsed | |||
}, | |||
theme: (state) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the theme getter used at all? You were accessing the theme with configuration.currentTheme
everywhere. If that's intended you might want to drop this getter entirely. Or map & use it instead of configuration
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks so awesome already ❤️
2a64d29
to
7951d7d
Compare
Could someone thoroughly test this with a locally linked owncloud/owncloud-design-system#1886 ? Rebased, animations fixed, extracted some things and fixed stuff the theme switch revealed in the ODS (so we'll need another release there) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things that I found in the UI while testing:
- (
search bar is gone?sorry for the noise, was in ocis on a page that has no filtering) - can we have a small right margin for the left sidebar, so that (when the app has a left sidebar nav) the app content gets a little more distance from the sidebar? At the moment it looks too compact IMO.
- when switching the theme the left sidebar has a slight delay / takes a little bit longer, compared to the rest of the UI
Dark mode:
- form input fields background color is a dark blue, I'd have expected something gray-ish (was looking at e.g. invite input field for people sharing and the page size dropdown in view options)
- OcDrop background same or too similar to the main background color, drop shadow exists but has no to little effect
- "+ New" button has black text on blue button but should have white text afaik
- the left sidebar nav items don't have the thick shadow on the right side of the nav icon
- OcSwitch background in enabled state has very little contrast to the main background color
- mediaviewer shows the label
1 of 3
in black instead of white - modals look broken
Light mode:
found nothing ✅
Edit:
- OcDrop styling is part of another story, if there are no easy fixes here, then keep it for later.
Thanks for testing @kulmann, did you try it with owncloud/owncloud-design-system#1887 locally linked? |
Yes 😇 |
✅
✅ rather added a padding to the router-view, right margin on the sidebar messed with the border-shadow
✅ added the same to the app content area now, please check if it's to your liking
✅ now has one of the "standard dark theme background colors", similar to the form field background in light mode
✅ not to resolve in this ticket, can't find the exalated ticket yet though
✅ should be resolved now
✅ resolved
owncloud/owncloud-design-system#1887, ✅ resolved by changing colors & adding a small border around it
✅ resolved
✅ resolved in the ODS, needed a background color
Commits for review incoming |
7951d7d
to
07ff96f
Compare
@kulmann what do you think in terms of a11y, add a tooltip&aria-label to the button and make the text aria hidden? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more minor things:
dark mode
- pagination has dark colors / no contrast
- notification bell has dark colors / no contrast
- search indicators in table header has dark colors / no contrast
- people invite form field has dark text color in autocomplete items (usernames and sharee type)
- OcSwitch color contrast is still bad
Yes, the sorting buttons. Would suggest to add a white outline to the icons, so that they are compatible with light and dark mode at the same time.
True, keep it like it is.
Same as for the sorting icons, add a white outline, so that it's compatible with light and dark mode at the same time. The knob in the middle is not that easily visible as well, though. |
e334466
to
280ee7c
Compare
280ee7c
to
c1be7b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests need fixing, but otherwise this is gold 🥇 happy to get it merged asap!
c1be7b4
to
cb7d04e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️ LGTM
Results for oCISTrashbinUploadMoveJourney https://drone.owncloud.com/owncloud/web/22016/66/1 |
cb7d04e
to
d90097c
Compare
Results for oC10iPhone1 https://drone.owncloud.com/owncloud/web/22017/47/1 |
d90097c
to
bf6b1a9
Compare
Kudos, SonarCloud Quality Gate passed! |
Description
This is currently WIP, but allows users to switch between a dark and light theme (provided this is found in the theming configuration). It initializes with the browser preference
Related Issue
Types of changes
Checklist: