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

Rename .flex-item-equal to .flex-1 #966

Merged
merged 6 commits into from
Nov 5, 2019
Merged

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Oct 31, 2019

This renames the .flex-item-equal utility to .flex-1.

Motivation

Currently when using .flex-auto and a sibling element is an image or icon, the image or icon can start to shrink. A workaround is to add .flex-shrink-0 to the sibling element.

Another workaround would be to use flex: 1 instead of flex: auto. It's similar but has a flex-basis value of 0%. It seems to be less "aggressive" and respect the min-width of sibling elements. And thus no .flex-shrink-0 is necessary on the avatar:

flex-1

Turns out that .flex-item-equal could be used. Because flex-shrink: 1 is the default, is has the same computed values as using the shorthand flex: 1 -> flex: 1 1 0% -> flex-grow: 1; flex-shrink: 1; flex-basis: 0%. Maybe it doesn't get used as much because:

  • It's only documented under Responsive flex utilities
  • Is longer to type than just flex-auto.
  • The name could be misleading when you only want one flex item to take up the available space.
  • It currently doesn't use !important and can't be used to override.

So renaming the utility might help. The recommendation would be:

  • Use .flex-1. In most situations it should be enough.
  • Use .flex-auto if a flex item should take up space based on its content. Add .flex-shrink-0 to sibling elements that are images or icons.

👀 See docs preview.

TODO

  • Remove .flex-item-equal
  • Add .flex-1
  • Update documentation

TODO in release-14.0.0

  • Deprecate .flex-item-equal class

TODO on dotcom

  • Replace .flex-item-equal with .flex-1

┌─────────────────────┬───────────┬─────┬───────────┬────────┬──────────┬─────────┬──────────────────────────────┐
│ name                │ selectors │   ± │ gzip size │      ± │ raw size │       ± │ path                         │
├─────────────────────┼───────────┼─────┼───────────┼────────┼──────────┼─────────┼──────────────────────────────┤
│ primer              │      3332 │   0 │   26.87 K │ - 17 B │ 168.89 K │ - 100 B │ dist/primer.css              │
│ core                │      2224 │   0 │    17.7 K │ - 17 B │ 112.26 K │ - 100 B │ dist/core.css                │
│ utilities           │      1360 │   0 │    8.62 K │ - 20 B │   65.6 K │ - 100 B │ dist/utilities.css           │

/cc @primer/ds-core

@vercel
Copy link

vercel bot commented Oct 31, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/primer/primer-css/puatm5zo9
🌍 Preview: https://primer-css-git-flex-utilities.primer.now.sh

@simurai simurai changed the base branch from master to release-14.0.0 October 31, 2019 09:13
@simurai simurai changed the base branch from release-14.0.0 to master October 31, 2019 09:17
.flex#{$variant}-auto { flex: 1 1 auto !important; }
.flex#{$variant}-grow-0 { flex-grow: 0 !important; }
.flex#{$variant}-1 { flex: 1 !important; }
.flex#{$variant}-auto { flex: auto !important; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing from flex: 1 1 auto; to just flex: auto; should have no effect since the computed value is the same:

Screen Shot 2019-10-31 at 8 00 39 PM

Comment on lines -48 to -49
flex-grow: 1;
flex-basis: 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replacing

.flex-item-equal {
  flex-grow: 1;	
  flex-basis: 0;
}

with

.flex-1 { flex: 1; }

should be ok, since flex-shrink: 1 is the default for flex items and flex: 1 gets computed as:

.flex-1 {
 flex-grow: 1;
 flex-shrink: 1;
 flex-basis: 0%;
}

@simurai simurai changed the base branch from master to release-14.0.0 November 1, 2019 02:24
Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

This is great. Thanks for documenting the initial values of the other flex properties; that makes me much more confident about rolling this out! There are only a couple dozen instances of flex-item-equal (and responsive variants; looks like just flex-md-item-equal) to replace in github/github.

@shawnbot shawnbot mentioned this pull request Nov 5, 2019
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants