okp4 / dataverse-portal

🔭 Dataverse Portal for the OKP4 network.
BSD 3-Clause "New" or "Revised" License
8 stars 1 forks source link

Enhancement/rework palette with design system colors #475

Closed loiclaudet closed 1 year ago

loiclaudet commented 1 year ago

Rework color palette based on design system colors.

See this discussion for more details.

fredericvilcot commented 1 year ago

@0xLodz For my own understanding, why the major part of the themed functions in stylesheets have been removed in favor of static variables?

loiclaudet commented 1 year ago

For my own understanding, why the major part of the themed functions in stylesheets have been removed in favor of static variables?

@fredericvilcot replaced themed functions in stylesheets has been removed because they were not themed related. The colors were the same, both on light and dark themes. That's why the functions were removed from theme file too, and directly used in stylesheets.

fredericvilcot commented 1 year ago

For my own understanding, why the major part of the themed functions in stylesheets have been removed in favor of static variables?

@fredericvilcot replaced themed functions in stylesheets has been removed because they were not themed related. The colors were the same, both on light and dark themes. That's why the functions were removed from theme file too, and directly used in stylesheets.

Yep, that's what I understood.

But I do not agree with that, the design should be scalable and extensible.

Here, stylesheets declare by themselves hardcoded colors, with no capability to be changed without modifying the file itself and thus, with no capability to provide another theme than the provided one.

To be more compliant with the theme design, the whole app color system should rely on the theme itself, even though the color are the same (at the moment) between dark and light theme.

By doing this, you can easily swap colors from the theme file, without editing any stylesheet.

And this is important because tomorrow, the app should offer the user the capability to instantly override the global style by providing its own theme variables (e.g through a JSON file).

loiclaudet commented 1 year ago

I do not agree with that, the design should be scalable and extensible.

Here, stylesheets declare by themselves hardcoded colors, with no capability to be changed without modifying the file itself and thus, with no capability to provide another theme than the provided one.

To be more compliant with the theme design, the whole app color system should rely on the theme itself, even though the color are the same (at the moment) between dark and light theme.

By doing this, you can easily swap colors from the theme file, without editing any stylesheet.

And this is important because tomorrow, the app should offer the user the capability to instantly override the global style by providing its own theme variables (e.g through a JSON file).

@fredericvilcot adding a new theme color name for each color, even if they are similar for light and dark, means that we need to search an appropriate name for each element color we will need, and add it into the theme file. It has a cost and impacts the developer experience, for a potential user that want to update the app theme through the theme file.

Theme depends on color palette, which is a good way of keeping consistency between various part of the app. Currently, user can still update a large part of the app via the palette.

I understand your point, your suggestion is probably the best for a maximum of flexibility, but do we really need it? If user from community ask for it, we can still make an update in that direction.

My suggestion would be to iterate only if it's necessary and required by the community, and keep it simple during the active app development duration. It makes the integration process faster, and avoid the theme file to grow.

What do you think?

fredericvilcot commented 1 year ago

@0xLodz I can't agree with that. 😌

We must only use colors by using the themed function, it's the only way to ensure that the app is thematized and thus could be branded according the user theme. The whole theme mechanism has been designed in that way.

If not, everything is static.

To bring more context and to highlight this point, you must be aware that there is an incoming feature in the pipe which consists in giving the user the capability to provide its own theme with its own mappings that will override OKP4 base theme.

As an idea, he could bring a JSON file as input and we will map its variables to the existing theme keys; so that is very important to declare explicit keys.

As an example, Docusaurus offers a base theme that you can extend or override by mapping your own style values to existing variables; that's the way we must follow.

So please, revert the themed functions everywhere needed, thanks a lot 😎