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
Sidebar improvements at smaller screen sizes #1436
Sidebar improvements at smaller screen sizes #1436
Changes from 10 commits
9693009
4a69db2
ed18313
5df316f
4d55656
9d885b0
ab00c53
210b1fa
6db3054
b292caa
d76f61d
0e01c66
5bbe19e
1278e73
df354bc
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.
With this being so close to the release, I'd rather use the existing
as_fill_item()
/as_fillable_container()
.There is also the subtle
copy()
issue here, which I know isn't a problem in the way you're currently using it, but it'd be very easy for someone use this in future and unknowingly mutate user inputThere 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 mean, the point is to avoid the unnecessary copy for elements we're creating internally. But OTOH I can see that it's worth avoiding the potential for mutating user input or having to document two separate approaches, plus it's only a shallow copy of the outer tag anyway. I'll go back to the existing functions.
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.
FTR, we actually make two copies because we need to call
as_fill_item(as_fill_carrier(tag))
and both make a copy.Personally, I'd definitely prefer what @schloerke recommended above: having
add_fill_classes()
for internal usage and also calling it directly inas_fill_item()
andas_fill_carrier()
. I think a reasonable docstring onadd_fill_classes()
would help with the mutating inputs issue.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.
Anyway, 5bbe19e reverts back to using
as_fill_item(as_fill_carrier(main))
Large diffs are not rendered by default.
Large diffs are not rendered by default.