search

josdejong / svelte-jsoneditor

A web-based tool to view, edit, format, repair, query, transform, and validate JSON
https://jsoneditoronline.org
Other
816 stars 108 forks source link

fix: scope theme variables #340

Closed SamMousa closed 7 months ago

SamMousa commented 7 months ago

Tested it via npm run dev and then doing a visual inspection.

This doesn't change the HTML structure, which is good, however it might break external theme modifications due to selector specificity. Ie someone having their own variables in :root vs us having the defaults in .jse-main. According to the specificity calculator (https://specificity.keegan.st/) it should be fine. Since specificity is equal it is just about ordering.

josdejong commented 7 months ago

Thanks Sam!

Can you also test the context menu (right click), the modals (sort, transform), and opening a nested editor in a popup (right click on an object -> edit object). I think those need something extra to get the CSS variables.

SamMousa commented 7 months ago

I've fixed it so that it works, but it is not ideal because it requires internal knowledge of which classes to target. I'd propose a different approach instead, with the following goals in mind:

Fixing the CSS targeting issue

  1. Define all default variables as SCSS variable
  2. Change all usages to use the SCSS variable value as fallback

This means that themes are no longer competing with default styles

Fixing non prefixed CSS variables

Since we cannot change the usage of non-prefixed CSS variables (they are in 3rd party code) this will need some extra work.

  1. Define a prefixed variant for each variable, this is treated just like the previous section (with defaults coming from SCSS variable)
  2. Define the svelte-select variables directly on the element by using its exported property containerStyles (https://github.com/rob-balfre/svelte-select#props)

I can make a PR for this, and the nice thing is, it'll not break backwards compatibility, whereas this PR in its current state does (since external themes will not properly be prioritized over the default theme.

josdejong commented 7 months ago

Thanks for diving into this Sam!

We indeed need to solve this, and the user shouldn't need to know about the internals. I.e. this example custom_theme_color should keep working.

It would be great if you can work out a solution for this, thanks!

It makes sense to introduce prefixed variants for the svelte-select variables.

I do not yet understand the need to define everything as default variables as SCSS to be used as a fallback for the CSS variables, can you explain that?

SamMousa commented 7 months ago

I do not yet understand the need to define everything as default variables as SCSS to be used as a fallback for the CSS variables, can you explain that?

Given the requirements:

Since you cannot set a CSS variable if not set, this is an issue. So instead of initializing our own CSS variables, we shouldn't do that at all. So instead of doing this:

:root {
   --jse-color: red;
}

And then using it somewhere like this:

<div class="header">Header</div>
<style>
.header {
    color: var(--jse-color);
}
</style>

We should / could be doing this:

$jse-color: red;
<div class="header">Header</div>
<style>
.header {
    color: var(--jse-color, $jse-color);
}
</style>

After confirming that we can use SCSS variables like this of course.

Another approach would be to use internal CSS variables and define them instead.

:root {
   --jse-internal-color: red;
}

And then using it somewhere like this:

<div class="header">Header</div>
<style>
.header {
    color: var(--jse-color, var(--jse-internal-color));
}
</style>

Hope that makes sense

josdejong commented 7 months ago

I understand what you mean, that makes sense indeed. Some thoughts:

  1. About maintainability: using var(--jse-color, $jse-color) internally everywhere will indeed work. It will be tricky though to never forget to define the second argument with the default value. I guess in that case we could define internal CSS variables like --jse-color-internal: var(--jse-color, $jse-color) and use this variable. But in that case it is possible to accidentally use --jse-color instead of --jse-color-internal, also not ideal 🤔. Or are there other options?

  2. We don't want variables leaking, so we cannot just target :root

    That is indeed the ideal solution. However if this leads to more complexity and a harder to maintain code base, we can also accept that the variables are globally defined, as long as they all have a clear --jse-... prefix to prevent naming collisions (the main goal). This may be a more pragmatic solution. What do you think?

SamMousa commented 7 months ago

Or are there other options?

I have not thought of other options yet...

This may be a more pragmatic solution. What do you think?

I did think of this option, but this actually will be very complicated for the case of svelte-select. If we prefix some of their variables people who have a set of svelte-select variables will have to selectively prefix it with --jse-ss- to work. Alternatively we introduce prefixed versions of all variables, but this is maintenance hell. For that specific case I'd argue it might be cleaner to just use the no-styles variant and sit down once to create a JSONEditor theme for it (where we don't support their variables for customization at all, but optionally introduce some of our own). I even started on a branch that just changed the prefixing of svelte select variables but felt that led nowhere... How do you want to handle 3rd party custom css properties?

In conclusion I think not leaking variables is good, and the issues you describe we can easily catch with code style tools. For example we could use https://stylelint.io/user-guide/rules/custom-property-pattern to enforce the correct pattern for all our files (forcing them to use --jse-prop-internal) and apply that to all files except the themes directory. FYI: I have not used this and am not sure how it'll work together with SCSS, but you get the idea.

josdejong commented 7 months ago

True, it should be possible to use some linting tools help using the variables the right way. Intuitively, the var(--jse-color, $jse-color) solution feels most simple to me than the --jse-color-internal: var(--jse-color, $jse-color) approach, but I'm not sure how it will work out for real.

Maybe the best approach is to give this a try and see how it works out (not having global variables).

I think svelte-select inside svelte-jsoneditor should align and use the styling of svelte-jsoneditor as much as possible (colors, padding, etc). Only thing is that svelte-jsoneditor should not expose the variables it sets for svelte-select globally. We can do something that boils down to .jse-main { --background: var(--jse-input-background) }. If starting from no-styles helps here (over tweaking the default styling a bit) I'm ok with that, but the default styling is very good and for svelte-jsoneditor I only needed to configure a few of the variables. Shall we leave that as it is for now to keep this PR focused on getting rid of the globals, and in a separate PR think though if we can improve how we style svelte-select?

SamMousa commented 7 months ago

No need to merge this PR btw, if we're redoing it as SCSS variables the default theme CSS file will disappear (or be transformed to an SCSS file).

SamMousa commented 7 months ago

Ohh, working on the PR now, found a really clean syntax. In default-theme-variables.css we define variables like this:

  $tooltip-color: var(--jse-tooltip-color, $text-color);
  $tooltip-background: var(--jse-tooltip-background, $modal-background);
  $tooltip-border: var(--jse-tooltip-border, $main-border);
  $tooltip-action-button-color: var(--jse-tooltip-action-button-color, $text-color-inverse);
  $tooltip-action-button-background: var(--jse-tooltip-action-button-background, #4d4d4d);

That is the most ugly part. But now at all usages, we only need to use the $tooltip-color variable. This means rules code become easy:

PR soon