Nord theme #1139
Nord theme #1139
Conversation
This commit adds nord theme to the list of themes (working only on 24bit mode). Nord theme was inspired by https://www.nordtheme.com/ Lacking 256 and 16 color modes.
|
@neiljp is everything ok? |
There was a problem hiding this comment.
@felipecpassos Thanks for looking into this - I've been away for a bit so just coming back to it now! 👍
You don't state this, but this is effectively a "dark ambiance" theme, to use the terminology on the Nord website?
If so, while the main Nord website doesn't seem to have much evidence of "white ambiance" designs in screenshots, it might be useful to use the _dark suffix for our theme like we have zt_dark and gruvbox_dark (which might eventually have the matching gruvbox_light, for example).
It's good to see the tests appear to run OK and manually the style looks to be rendering visually OK after a quick check, though I've not examined whether the nord colors match the recommended usage in detail.
For the color specification, for easier comparison with the source material, and also referencing within the styles, I feel it would be cleaner to order the NordColor entries in the same order as in their documentation, as well as use the descriptive prefixes eg. maybe FROST_7 and AURORA_ORANGE_12.
There are a few extra colors which aren't in the Nord palette - It may be challenging but I'd hope that we'd be able to avoid using them?
neiljp
added
the
PR awaiting update
About the color names, i gave them the same name from Nord pallete:
I called that color NORD_7. Would you prefer me to call it FROST_7 or FROST_1 ? |
felipecpassos
force-pushed
the
nord-theme
branch
from
325c0a6 to
5f4e8dd
Compare
There was a problem hiding this comment.
@felipecpassos I added a few notes based on the changes you made already.
Re the numeric suffix, I used the <n> from nord<n> as a suffix since that's how the color is referenced in that document. I can see it being beneficial both ways. In my AURORA example I used ORANGE, so for the others we could include a primary index instead of a subtle color-name, so my examples might become FROST_1_n7 and AURORA_ORANGE_n12. Those last digits are the nord index, which we could also potentially list in an aligned comment at the end of each line, as they won't line up in the names.
For merging note that we'll likely want to squash the commits together manually before merging due to the rebase-oriented workflow we use.
| @@ -86,7 +86,7 @@ | |||
|
|
|||
| THEMES = { | |||
| "gruvbox_dark": gruvbox, | |||
| "nord": nord, | |||
| "nord_dark": nord, | |||
There was a problem hiding this comment.
This is fine until we have a light version :)
| @@ -95,6 +95,7 @@ | |||
| THEME_ALIASES = { | |||
| "default": "zt_dark", | |||
| "gruvbox": "gruvbox_dark", | |||
| "nord": "nord_dark", | |||
There was a problem hiding this comment.
This section was intended for historical aliases; I'd prefer we avoid supporting "nord" as a theme name for now, which is what this line would do.
|
@felipecpassos Do you need more support with this? |
|
Heads up @felipecpassos, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
|
@felipecpassos This will need a little more work after a few changes to styles, and I'd recommend for you or anyone else taking this up where there may be multiple versions (eg dark/light) to split the common color palette into a separate file, as we've now done for gruvbox. |
What does this PR do?
Added support for Nord theme.
Partial fix for #1133
Tested?
Commit flow
Notes & Questions
Interactions
Visual changes