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

feat: Add setTheme to custum cozy bar design #451

Merged
merged 10 commits into from
Feb 18, 2019
Merged

feat: Add setTheme to custum cozy bar design #451

merged 10 commits into from
Feb 18, 2019

Conversation

kosssi
Copy link
Contributor

@kosssi kosssi commented Feb 11, 2019

Change theme bar

It's possible to update design on the cozy-bar with setTheme function.

const { setTheme } = cozy.bar
setTheme('default')
setTheme('primary')

This feature impact only mobile version.

default primary
capture d ecran 2019-02-11 a 16 58 31 capture d ecran 2019-02-11 a 16 58 12

@kosssi kosssi self-assigned this Feb 11, 2019
@kosssi kosssi requested a review from CPatchane as a code owner February 11, 2019 15:51
@edas
Copy link
Contributor

edas commented Feb 11, 2019

Instead of

html {
  --primary: var(--dodgerBlue)
} 
[role=banner] .coz-bar-wrapper {
  background-color: var(--white)
}
[role=banner] .coz-bar-wrapper.theme-primary {
    background-color: var(--primary)
}

Shouldn't we have something like :

html {
  --primary: var(--dodgerBlue)
  --background: var(--white)
} 
[role=banner] .coz-bar-wrapper {
  background-color: var(--background)
}
[role=banner] .coz-bar-wrapper.theme-primary {
    --primary: var(--white)
    --background: var(--primary)
}

?

Copy link
Contributor

@CPatchane CPatchane left a comment

Choose a reason for hiding this comment

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

Looks good but need more tests to be great 👍

src/components/Bar.jsx Outdated Show resolved Hide resolved
src/lib/reducers/index.js Outdated Show resolved Hide resolved
test/components/__snapshots__/Bar.spec.jsx.snap Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je pense que c'est problématique d'avoir les SVG avec de la couleur on devrait pouvoir avoir la même SVG pour les 2 themes. Mais c'est à voir dans de l'optimisation dans un futur plus ou moins proche...

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try to use the fill css property? Or maybe a SVGr component like for Home icon?

Copy link
Contributor Author

@kosssi kosssi Feb 12, 2019

Choose a reason for hiding this comment

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

It's more complex because we used it on background-image.

With my comment I wanted to warn you but not necessarily solve the problem in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can avoid using it as a background-image? Did you try it?
This issue should be solved in this PR instead of keeping it in the code for a while IMHO

Copy link
Contributor

Choose a reason for hiding this comment

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

@CPatchane I used an Icon from cozy-ui instead of the background-image svg. Is it OK for you like this?

@drazik drazik merged commit a4b1286 into master Feb 18, 2019
@drazik drazik deleted the theme branch February 18, 2019 13:27
@cozy-bot
Copy link

🎉 This PR is included in version 6.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants