-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Style updates #1350
Style updates #1350
Conversation
This approach generally looks good to me @pngwn. I skimmed through the components and it looks like we're providing users with a good set of initial styling parameters to start with. We'll need to add more over time as users request them, but they should already be able to go pretty far with this. I'll let @aliabid94 and @gary149 speak more on the actual frontend changes. With regards to the backend, lets make sure to throw deprecation warnings when we're moving parameters (I created an example -- can create the others once everything else is finalized) |
Added deprecation warnings and reimplemented the color_maps where appropriate. Resolved conflicts. |
Interesting, I think this makes sense for the most part. the whole variant as "info", "warning", etc. system reminds me of bootstrap which doesn't allow that much flexibility, but I guess theming is a different discussion. I'm thinking if there are other style properties we should add - rounded and container stuff is pretty limited customization. |
@aliabid94 The focus here is essentially just for things people might need to modify to get different layouts. I think style customisations across the board should be handled via themes (indluing scoping 'themes' to invidual component instances or providing theme value overrides for individual component instances). This why i removed the colors specifically (mainly because dark mode makes defining colours really difficult). I actually went through and looked at every component in detail and the margin setting basically didn't do anything and really couldn't, nothing has margins for the most part. I made sure to strip any margin or padding when I think it is better to strip things down for now because it is far easier to add stuff than to remove. The options we had (many of which didn't make sense or didn't do anything) are a good example of what will happen if we just add things on a case by case basis. We need to look at styling/ theming holistically and not just add things piecemeal, or i think it will get out of hand and give unexpected results. |
Merging before there are any more conflicts. |
This PR introduces some style changes to ensure that user can't completely break theirs app with the options that get passed in. There are also a few changes to Simplify how certain things are handled. Other changes are simply hooking
Box
to have a few variants, maybedefault
,info
,warning
anderror
each with their own background style but that works well for both luight and dark mode. The colors for these 'types' would be customisable with the themeing system (that doesn't yet exist).color_map
still exists where it is used and is unchanged, i have moved this kwarg from the component to the style method. The rationale here is that anything that only affects the visuals should be a style kwarg.show_label
is a bit of an anomaly here but it feels kindof logical. We could move it as well though.container
is a style kwarg now, same logic as above.container_bg_color
has been removed. Instead of allowing people to modify the container in this way (and end up with lots ofcontainer_*
styles), we should encourage users to setcontainer=False
and usegr.Box()
which they can pass some styles too.margin
, only a few components actually have margins, most of the time the margins are actually paddings and i think we can remove them when thecontainer=False
. But that means we need a way to add them back, I'm not sure what that should be.rounded
also gets a little confusing. I tried to make it so that theround
is relating to whatever the main part of the component is but it is a little confusing. On form inputs (Textbox, Number, Checkbox, etc) it relates tot he input itself, this looks fine on most but terrible on checkbox + radio. Onfile upload components, this actually refers to the containing box which makes sense (ish) because that is the component but is also slightly inconsistent with the fporm inputs. For table I'm not sure if this will break anything.note: there will probably be loads of conflicts when some of the current PRs go in. This PR is unfinished, I've been back and forth on a number of options and wanted to get other people's thoughts before I continued. There isn't much left to do in terms of hooking the last few styles up in the frontend
Tech stuff
I also refactored the way in which styles are applied in the frontend but that is an implementation detail. Happy to go through it @aliabid94. Some inert code still needs removing.