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

Experimental style changes #110

Closed
wants to merge 9 commits into from
Closed

Experimental style changes #110

wants to merge 9 commits into from

Conversation

maun
Copy link
Contributor

@maun maun commented Sep 26, 2023

I explored the code base and tried some things

Style simplification

Style (previously ComputedStyle) holds the concrete values (no StyleValue), and StyleFns are just closures which modify the style.
So starting with Style::default()all boxed StyleFns are used in the following order, this is mostly as before:

  • base style
  • style
  • responsive styles
  • hover style
  • focus style
  • active style
  • disabled style
  • animation system changes
  • override style (new)

This could also have been done with Option instead of StyleValues, but I originally planned an API like .style([padding(10),flex(), ...]), where each is a style modifying closure. Could also still change it to be like this.

I must admit I do not really know whether this is better, we loose the ability to unset styles, but everything seems to still work as before. Also it is a bit simpler, and fewer lines of code.

Style Units

I created newtype wrapper Px for pixel values and Pct for percent values. Px is chosen by default with multiple From<x> implementations. Style functions which can work with both take an enum (impl Into<PxOrPct>).
Instead of style functions like width_px and width_pct there is now only width

Short style functions

I added some shorter aliases like p for padding, mx for margin_horiz, and so on. Inspired by tailwind.

Multiple box shadows

Just as in CSS (still missing inset styles). This required a different style API.

I realize these are more controversial changes, no pressure to merge as is. If you want some of these changes I can rollback the rest, or create individual PRs. Also not all comments and examples are updated, I wanted to open the PR first.

I plan to have a look at the animation system, and change some things. At least I have fun coding this, thanks for the project.

@dzhou121
Copy link
Contributor

The reason we went with StyleValues over Option is because of the ability of unset values which is quite important.

I like the change of style units. But maybe we should keep things like width_pct(100.0) because it's so much easier to input than width(Pct(100.0))

@maun
Copy link
Contributor Author

maun commented Sep 26, 2023

The reason we went with StyleValues over Option is because of the ability of unset values which is quite important.

How is this used? I did not see it.

I like the change of style units. But maybe we should keep things like width_pct(100.0) because it's so much easier to input than width(Pct(100.0))

I can make a PR for this 👍

@dzhou121
Copy link
Contributor

@MinusGix Any use case you can think of for unset?

@maun
Copy link
Contributor Author

maun commented Sep 26, 2023

I like the change of style units. But maybe we should keep things like width_pct(100.0) because it's so much easier to input than width(Pct(100.0))

Do you like width(100.pct())?
This way combining is easier: size(100.px(), 50.pct())
No uppercase and autocomplete for the parentheses, also in the same order as one thinks / spells it.

@dzhou121
Copy link
Contributor

@jrmoulton How do you like width(100.pct()) size(100.px(), 50.pct())?

@jrmoulton
Copy link
Collaborator

@jrmoulton How do you like width(100.pct()) size(100.px(), 50.pct())?

I like it but I don't like that you have to import the extension trait every time you want to use it.

It's also looks more noisy than using functions the _px functions but when I look at it with syntax highlighting it actually looks nice.

width(100.pct()); 

size(100.px(), 50.pct())

Even though I've never needed it, it's nice that it's easier to mix and match units.

Hmmm. I think I'm in favor of it but the issue of needing to import the extension trait seems annoying. Maybe worth it though

@maun
Copy link
Contributor Author

maun commented Sep 27, 2023

Closing, style unit changes are in #112

@maun maun closed this Sep 27, 2023
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

Successfully merging this pull request may close these issues.

3 participants