feat: use SCSS variables internally

[SamMousa]

This reworks the default theme to use SCSS variables.

• All variables are defined in our defaults.scss file.
• SCSS variables use a CSS custom property and fall back to a default value.
• Other code MUST only use SCSS variables.

Some things that are still todo:

• [x] Fix getIndentationStyle() in CollapsedItems.svelte
• [x] Fix getIndentationStyle() in JSONNode.svelte
• [x] Fix codemirror theme that uses a style definition in javascript.

Since these todos only require a very small subset of variables one solution is to define them in the relevant component as internal CSS variables.

Todo from feedback below: Using the default theme:

• [x] the keys in text mode are red instead of black
• [x] the light gray border of the editor is gone
• [x] in transform modal: border of the preview editors is missing

the selection bar between two rows:

• [x] is missing the left-margin
• [x] when the editor does not have focus, it is not rendered correctly (shoud be rendered in light gray) in the nested editor modal (right click an object, click "Edit object" in the context menu):
• [x] editor menu is missing the background color
• [x] the Apply button bottom right is missing a background color

Using theme "dark":

• [x] the keys in text mode are light green instead of light blue (same issue as default theme?)
• [x] the select boxes do not have a dark theme in the Transform Modal and Sort modal

In the nested editor modal (same as with the default theme I think):

• [x] the menu doesn't have the right background color and text color
• [x] the Apply button is missing a background color
• [x] the validation warnings a the bottom do not have a dark text color

Using theme "big"

• [x] the select boxes in Transform and Sort modal do not respect the font-size

[SamMousa]

@josdejong rudely pinging you!

This PR solves the styling issue:

• [x] No more leaking variables
• [x] Svelte-select variables that are also styled by JSONEditor must be overriden using --jse-svelte-select prefix.
• [x] Usage of values in code is simple
• [x] One definition file that contains the SCSS variable definitions with defaults and which are overridable using CSS custom properties

I hope you like it, otherwise I wasted a few hours for nothing xD

[josdejong]

Thanks a lot Sam, it looks neat at first sight! I'll do an in-depth review+testing coming Wednesday I expect.

One first thought: I think the file jse-theme-dark.css should not contain SCSS variables and be plain CSS, so I think $text-color should remain var(--jse-text-color) like it was before?

[SamMousa]

Yes, that might be a global search and replace mistake. Thought I caught all of them..

[josdejong]

I see there where some tricky things involved, like getting the colors work in text mode, and you did something nice with the calculation of the left margin of nested items. Great job!

This is a lot more work than the original 1 line PR 😅 , but I really love how this works out. It is indeed easy to maintain like this.

I've gone through all details of the editor, and almost everything looks alright! I came across a number of (I think) small issues, listed below. Can you have a look into those? Please let me know if you need help.

I've mostly tested with the development page http://localhost:5173/development:

Using the default theme:

1. the keys in text mode are red instead of black
2. the light gray border of the editor is gone
3. in transform modal: border of the preview editors is missing
4. the selection bar between two rows: 4.1. is missing the left-margin 4.2. when the editor does not have focus, it is not rendered correctly (shoud be rendered in light gray)
5. in the nested editor modal (right click an object, click "Edit object" in the context menu): 5.1 editor menu is missing the background color 5.2 the Apply button bottom right is missing a background color

Using theme "dark":

6. the keys in text mode are light green instead of light blue (same issue as default theme?)
7. the select boxes do not have a dark theme in the Transform Modal and Sort modal
8. In the nested editor modal (same as with the default theme I think): 8.1. the menu doesn't have the right background color and text color 8.2. the Apply button is missing a background color
9. the validation warnings a the bottom do not have a dark text color

Using theme "big"

10. the select boxes in Transform and Sort modal do not respect the font-size

[SamMousa]

I've fixed some points, others remain open; I'd love some help to pull this over the finish line.

Regarding the buttons in modals I think the way this is currently implemented should actually be configured a bug:

1. Variable --jse-theme-color sets the primary theme color at the top level
2. Variable --jse-modal-theme-color sets the primary theme color for use inside modals
3. Variable --jse-button-primary-background is defined as var(--jse-theme-color

Due to the way CSS variables work the variable defined in 3 has the value from 1, even if it is shown inside a modal. This seems to be a reasoning error. Inside a model you have redefined the primary theme color so the button should obey that. This screenshot from jsoneditoronline.org shows the inconsistency with respect to the menu bar, which also by default has the theme color.

[josdejong]

Thanks for all the fixes. I'll look into the issue with the modals, no problem.

[SamMousa]

Big tip: if you see $variable in developer tools it's an issue with SCSS escaping. On the right hand side of CSS variable declaration SCSS requires explicit interpolation: --jse-abc: #{$abc} instead of the default implicit interpolation: background-color: $abc;

[josdejong]

Regarding the buttons in modals I think the way this is currently implemented should actually be configured a bug

Your explanation is correct. However my intention was a bit different: I wanted to be able to just give the menu of the editor inside a modal a different color because it was looking ugly to me. So I think the name --jse-modal-theme-color is misleading and should be something like --jse-modal-editor-menu-background-color. Renaming would be a breaking change, but we can keep it backward compatible I think. Does that make sense?

[SamMousa]

I wouldn't worry about BC at this level of detail to be honest. I mean anything can be considered a BC break if you're critical enough. This PR already breaks BC because we no longer support overriding some specific svelte-select settings via the original CSS variables, you have to use the --jse-svelte-select prefixed version for some of them.

[josdejong]

Good point, it's already a very breaking change 😂 . I'll publish it as a major release and explain the changes.

[SamMousa]

While you're at it you could also document the BC promise. For example you could say that users customizing svelte-select should ALSO use the prefixed variables. This allows you to customize those properties in the future without it being another BC break.

[josdejong]

Just for clarity: do you want me to finish the last open issues?

[SamMousa]

Just for clarity: do you want me to finish the last open issues?

yes please!

[josdejong]

okido

[josdejong]

I created https://github.com/josdejong/svelte-jsoneditor/pull/350, fixing the last open issues. I want to do one more testing round later this week, I expect on Wednesday.

@SamMousa one last question: do you know whether the new way to handle indentation with a CSS variable --level has any performance implications? I'm a bit in doubt whether CSS variables are suitable to be used like this, with possibly thousands of re-definitions with every re-render.

[SamMousa]

I can't imagine it is faster to change styles directly, since you're doing 1000s of updates in such a case as well... But this is a guess

[josdejong]

I just did a small test by hand, it actually feels a bit faster 😁. I didn't do a real benchmark but it is as fast as before or a bit faster.

[SamMousa]

That's what I expected, in theory it is more optimizable:

• the dependency graph for the custom property doesn't change, so it might have a list of all elements that depend on some property
• the CSS engine is optimized for calculating cascading values of properties, including custom properties

[josdejong]

It makes sense indeed. This is a nice side-effect of this refactor 😎👍

[SamMousa]

Btw, for future reference: you can directly push to a PR branch as well, as long as a contributor has checked the checkbox during pr creation. Thanks for your efforts in finishing it!

[josdejong]

Ahh, I didn't know that. That's very handy indeed, also for example for PR's that are about finished but just have a linting error or something like that.

[josdejong]

Btw, for future reference: you can directly push to a PR branch as well, as long as a contributor has checked the checkbox during pr creation.

Yay, that indeed works 😎

I've done another testing round, found a few small things and cleaned up some left over TODO's.

Time to merge this PR.

[josdejong]

The refactoring is now published in v0.20.0. Thanks a lot for all your effort and for thinking along Sam!