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

fix(css): elaborate how gap percentage values behave #34018

Merged
merged 11 commits into from
Jul 4, 2024

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner June 8, 2024 12:26
@OnkarRuikar OnkarRuikar requested review from bsmth and removed request for a team June 8, 2024 12:26
@github-actions github-actions bot added Content:CSS Cascading Style Sheets docs size/s [PR only] 6-50 LoC changed labels Jun 8, 2024
Copy link
Contributor

github-actions bot commented Jun 8, 2024

Preview URLs

External URLs (1)

URL: /en-US/docs/Web/CSS/gap
Title: gap

(comment last updated: 2024-07-04 04:09:53)

@OnkarRuikar OnkarRuikar marked this pull request as draft June 20, 2024 15:34
@OnkarRuikar OnkarRuikar marked this pull request as ready for review June 21, 2024 07:02
@github-actions github-actions bot added size/m [PR only] 51-500 LoC changed and removed size/s [PR only] 6-50 LoC changed labels Jun 21, 2024
@OnkarRuikar OnkarRuikar requested a review from bsmth June 21, 2024 07:03
Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
Copy link
Contributor Author

@OnkarRuikar OnkarRuikar left a comment

Choose a reason for hiding this comment

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

Let's make dimensions 24px to make flex and grid look same.

files/en-us/web/css/gap/index.md Show resolved Hide resolved
files/en-us/web/css/gap/index.md Show resolved Hide resolved
@OnkarRuikar OnkarRuikar requested a review from bsmth June 28, 2024 12:50
@bsmth
Copy link
Member

bsmth commented Jul 1, 2024

It feels a little wonky to use 24px here instead of 20px as with the others. What do you think about this:

https://codepen.io/bsmth/pen/mdYNyJb

i.e.:

body > div {
  background-color: #ccc;
  width: 200px;
  height: 200px;
  flex-flow: column;
}

#grid {
  display: inline-grid;
  gap: 12.5%;
}

#flex {
  display: inline-flex;
  gap: 12.5%;
}

#grid > div,
#flex > div {
  background-color: coral;
  width: 20px;
  height: 20px;
}

Or with four items and some even values

<span>Grid</span>
<div id="grid">
  <div>1</div>
  <div>2</div>
  <div>3</div>
  <div>4</div>
</div>
<span>Flex</span>
<div id="flex">
  <div>1</div>
  <div>2</div>
  <div>3</div>
  <div>4</div>
</div>
body > div {
  background-color: #ccc;
  width: 200px;
  height: 200px;
  flex-flow: column;
}

#grid {
  display: inline-grid;
  gap: 20%;
}

#flex {
  display: inline-flex;
  gap: 20%;
}

#grid > div,
#flex > div {
  background-color: coral;
  width: 20px;
  height: 20px;
}

@OnkarRuikar
Copy link
Contributor Author

It feels a little wonky to use 24px here instead of 20px as with the others. What do you think about this:

Yes, 20px is easy to explain.

@bsmth
Copy link
Member

bsmth commented Jul 2, 2024

Yes, 20px is easy to explain.

Looks better!

OnkarRuikar and others added 2 commits July 2, 2024 17:56
Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
@bsmth
Copy link
Member

bsmth commented Jul 3, 2024

I'm so sorry for dragging out this review... I've read through the description again and it can do with a reword because it's already describing percentage values. This branch has a suggested edit: https://github.com/OnkarRuikar/content/compare/css_gap...bsmth:content:css_gap_description?expand=1

Then the paras have these functions:

## Description

* what it is 
* how it works / looks
* use in grid
* use in flex
* use in multi-col
* percentage values
* history

I'm happy to merge after this!

@OnkarRuikar
Copy link
Contributor Author

I've read through the description again and it can do with a reword because it's already describing percentage values.

Yes, the text made sense when it was a separate section. Now in the description section, it looks repetitive.

I've done the changes.

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Looks super. Thanks for your patience, Onkar!

@bsmth bsmth merged commit b539052 into mdn:main Jul 4, 2024
8 checks passed
@OnkarRuikar OnkarRuikar deleted the css_gap branch July 5, 2024 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:CSS Cascading Style Sheets docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interpretation of percentage values in grid-gap
2 participants