-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
Theming initial proposal #4579
Theming initial proposal #4579
Conversation
✅ Deploy Preview for koda-nuxt ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@roiLeo @yangwao @vikiival @Jarsen136 @petersopko @preschian Let me know if this seems okay to you, or you would prefer some other way of doing it? I have some time because of the holidays going on, I can do this transition, I feel like it would speed up our development by doing this once and for all |
I really agree to find some smart way to make the theme colour switch more smoothly. For example, in the
|
yeah this is the most common approach I found as well |
@prachi00 hey, I really like your initiative and I am pretty sure we can reward this one! This PR can still be ongoing. |
Sure, this look good. |
yeah I guess we can do that, we just have to define a different object in theme.scss once the migration is done |
@exezbcz can you provide a list of colours and their dark-mode alternative colour? that would help a lot |
Have you search in figma files? |
I'm sorry for the delay. To make the colors more understandable, I will add more details - maybe in form of short description. You might have a look at landing page handoff ( first page) in the meantime. |
okay, from figma it isnt that clear, as not all the colors seem to have a dark mode equivalent (for example whats dark mode equivalent for k-accentlight or for k-hovergrey), it would be helpful if we could keep the variable name same and then have two different colors for it, one for light and one for dark. |
@roiLeo also, since we no use oruga, will we be eventually moving out of using buefy? |
sorry I can't find the related issue on Nuxt3/Oruga-ui migration |
@roiLeo @yangwao I updated this PR with changes to links and have added colors I thought were right to themes.scss , I'll update it once @exezbcz provides the color documentation as well |
IMO, CSS variables would be easier to use and also be more readable in the codebase. WDYT @roiLeo @prachi00 |
Sure should work too, we have already implemented something like this in nft-gallery/libs/ui/src/scss/variable.scss Lines 16 to 22 in 6223a9d
but I remember posting a comment with @preschian because I was stuck on a case where I had to override variable Could you open a small PR with your poc? maybe start by tweaking colors on edit: I think it was because I was trying to set sass variable with css variable or vice versa |
Yes, this part using CSS variables is what I have implemented in https://github.com/kodadot/nft-gallery/pull/4549/files.
There is nothing special to override some properties of css with css variables.
If we would like to override it, we could do something like this. Or simply replace the
If the problem is still there, I would like to take a look. |
How will you handle/override default bulma var? (in
|
To my knowledge, bulma var could only be overridden to a specific value. I guess we should not try to convert it into different colors accordingly. If a component using |
I don't think so we'll need to rewrite every scss component file that use |
You're right. With this approach, default scss variables could be overridden properly. |
soooo which approach are we following? |
Yours I think since we won't be able to override default bulma vars with custom css |
so can we merge this and make sure people follow this in the future prs and not add anything to dark and light scss files? |
@prachi00 maybe set this PR to ready for review, so the others can start reviewing |
@preschian @roiLeo @Jarsen136 can we please merge this so I can continue with this further |
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.
It looks strange to use, but if it's ok for @kodadot/code-review-guild I'm fine with that.
I see that there are still a lot of changes, I would have liked to try to overload the bulma variables first, fingers crossed that nothing breaks 🤞
The usage of mixin css looks strange also to me. I would prefer to use CSS variables to override custom colors(k-dark\k-grey), which would be easy to use and read. The mixin method is also necessary to override the oruga variables(like $primary). For now, let's merge it until we find a more property approach to handle it. |
They are mostly the same The tricky onesaccentlight - for button hover in light mode dark mode alternatives:
OthersThere are not many other variants, then there are few grey colors: k-grey, usually used for dividers, second priority text. hover on text: k-darkmode:
k-black
Then there is a single type of blue and a lighter variation that is only rarely used. those are the main colors, its only few of them because i did not really need much more. Once i finish redesigning, i will try to put it together somehow and make a better design system than this. Now the naming sucks a bit - i know it and i am sorry for that. Fortunately we have option to see which color was used in figma, you can check that as well if you dont. All the colors can be seen in landing page handoff first page. I usually update it when i introduce new color. You can refer to that if something is unclear. Thanks! |
if i understand this correctly, changing the hex codes and the names of the colors should be easier now? |
yes it should be |
Code Climate has analyzed commit 6b18442 and detected 0 issues on this pull request. View more on Code Climate. |
yeah I'll be making the changes after merging it to avoid conflicts and we have to make sure that all the theming issues follow this approach now |
Thank you for your contribution to the KodaDot NFT gallery.
👇 _ Let's make a quick check before the contribution.
PR Type
Context
Before submitting pull request, please make sure:
Optional
Had issue bounty label?
Payout
Community participation
Screenshot 📸