Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Tabs widget and the Rotated widget that it uses. #1160
Tabs widget and the Rotated widget that it uses. #1160
Changes from 13 commits
fe395db
bfccbfa
7967e05
86ddd32
6ccedde
ab52e7b
819aa27
5aad9cf
028f1ab
068c715
8246e06
a352942
9826b58
cc29a72
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when grouping items in
Flex
containers, I would like to discourage having explicit padding on each item and instead use thewith_default_spacer
method to insert space between items. Playing around I would add that between eachwith_child
, and also after each child that contains aRadioGroup
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check that, not sure I knew about it when I wrote this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying it, this seems to increase the code noise of layout noise quite a bit, because factoring it out is harder (multiple calls onto a flex instead of a returned value ), meaning cut and paste is more likely.
Why is the spacer better than padding, or why does it matter? Also it isn't doing the same thing is it? Padding is on all edges, and the spacer is only in the direction of the flex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is ensuring widgets look correct when used beside one another; this is especially an issue when using a
RadioGroup
. If you use thedefault_spacer
methods, you will have the same space between widgets as is used inRadioGroup
(and other built-in components) wheras if you pick your own padding values you'll end up with items that don't line up correctly.This isn't currently a practical issue in this example, but given that examples in general tend to be copied by new users, I want to discourage people from using arbitrary padding values, and encourage them to use these other mechanisms that will create more consistent layouts.
I agree about the increased code noise, and don't have a good answer there. Perhaps in the future we will apply auto-padding to items in collection by default or something? But this is where we are for the time being. 😒