Skip to content
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

why keeping json split files for the colors ? #575

Open
12rambau opened this issue Aug 22, 2022 · 2 comments
Open

why keeping json split files for the colors ? #575

12rambau opened this issue Aug 22, 2022 · 2 comments

Comments

@12rambau
Copy link
Member

I haven't seen these changes before, and I want to submit a comment.
I understand the advantage of having the style files organized in different folders, especially javascript and css... what is not totally clear for me are the JSON files.
I saw that we are updating some of the properties directly after calling the objects where are going to be used, to be sure they're adapted to the current theme (as here: https://github.com/12rambau/sepal_ui/blob/a6836ec2c4e25386d2167379f950a9fbe08fc4c0/sepal_ui/mapping/map_btn.py#L30).
So IMO I think that we are creating an extra step within the implementation. We have created a color Box to deal with the colors, wouldn't be useful do the same with the json files?
@12rambau

Originally posted by @dfguerrerom in #533 (review)

@12rambau
Copy link
Member Author

12rambau commented Feb 8, 2023

That's an old one so I wanted to get rid of this issue. some reasoning about this:

  • css-like information are no more (so the map_btn example is not adapted anymore
  • I like to keep thing separated so I don't like keeping big dict in python files like the one for the icons. I find it easier to read, manipulate and split static data from the code (you can argue against this one as the colors are still store in the frontend py files)
  • The color of the progres_bar should be moved to color that's effectively useless
  • I like to have splitted json files for layers style as it's a pure dict and out of 8 parameters we are only changing 2. What we could discuss I think is merging them in a 1 single layer.json (easy reading). keeping the color there is according to mee a must do to make sure you can use them out of the box.

let me know what you think

@dfguerrerom
Copy link
Collaborator

dfguerrerom commented Feb 9, 2023

I don't get what you mean with:

css-like information are no more (so the map_btn example is not adapted anymore

Ok, let's say we want to keep them in different files, in the end that's not an issue, what I believe is making this process more complicated is instead of just from sepal_ui.frontend.styles import layer_style and use it, we are doubling efforts import json dir>read as text>update color... what about creating a .py wrapper to read .json files and styles? or just use the styles.py file?

And, yes, I remember I posted this issue mainly to the fact of having different files for the same thing, "layer", so I would say I'm positive to left all the layer related styles in the same file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants