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

Add default horizontal & vertical padding to theme #1220

Merged
merged 1 commit into from
Sep 16, 2020
Merged

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Sep 11, 2020

This also uses these values in the Flex container, via new
add_default_spacer and with_default_spacer methods. This should
be the preferred way of spacing the standard control widgets.

This is progress on #1216; I also want to go and remove any other
internal padding constants from existing widgets.

@cmyr cmyr added the S-needs-review waits for review label Sep 11, 2020
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good idea.

Maybe with_theme_spacer would be a more obvious name? Because I have no concept of a "default spacer" but I would immediately know what's meant by a "theme spacer".

@luleyleo luleyleo added S-conflict blocked by merge conflicts and removed S-needs-review waits for review labels Sep 12, 2020
@cmyr
Copy link
Member Author

cmyr commented Sep 12, 2020

I was thinking about making this with_spacer and making with_spacer be with_fixed_spacer, but that would be annoying breakage. maybe with_auto_spacer?

@luleyleo
Copy link
Collaborator

Hmm, I've the same problem with with_auto_spacer as with with_default_spacer, actually I've even more of an issue with 'auto' because it makes me think of something adaptive...

Actually, now that I think about it again, with_default_spacer seems fine to me. Calling it with_theme_spacer makes no sense if the user (or even Druid) would define multiple different "default" sizes.

Maybe we could call them with_spacer and with_custom_spacer?. I don't think changing with_spacer would be an issue, as it is a single find-and-replace to fix it.

This is one of the rather rare moments where I wish Rust had overloading...

This also uses these values in the Flex container, via new
add_default_spacer and with_default_spacer methods. This should
be the preferred way of spacing the standard control widgets.

This is progress on #1216; I also want to go and remove any other
internal padding constants from existing widgets.
@cmyr
Copy link
Member Author

cmyr commented Sep 16, 2020

I'm not sure there's anything I like better than with_default_spacer, so I think I'm just going to keep that as-is for now 🤷

@cmyr cmyr merged commit 661b309 into master Sep 16, 2020
@cmyr cmyr deleted the standardize-padding branch September 16, 2020 20:55
@xStrom xStrom removed the S-conflict blocked by merge conflicts label Jan 17, 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.

None yet

3 participants