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.

Sign up for GitHub

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

Jump to bottom

fix(css): explain CSS aspect ratio comparison #37509

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix(css): explain CSS aspect ratio comparison #37509

wants to merge 6 commits into from
+33 −14

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar requested review from a team as code owners January 6, 2025 03:21
@OnkarRuikar OnkarRuikar requested review from estelle and chrisdavidmills and removed request for a team January 6, 2025 03:21
@github-actions github-actions bot added Content:CSS Cascading Style Sheets docs Content:Glossary Glossary entries size/s [PR only] 6-50 LoC changed labels Jan 6, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 6, 2025 •
edited
Loading

Preview URLs

Flaws (96)

Note! 1 document with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/CSS/@media/aspect-ratio
Title: aspect-ratio
Flaw count: 48

  • macros:
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/CSS
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/Getting_started_with_the_web/CSS_basics
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/CSS/First_steps
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/CSS/First_steps/What_is_CSS
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/CSS/First_steps/Getting_started
    • and 43 more flaws omitted

URL: /en-US/docs/Web/CSS/ratio
Title: <ratio>
Flaw count: 48

  • macros:
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/CSS
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/Getting_started_with_the_web/CSS_basics
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/CSS/First_steps
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/CSS/First_steps/What_is_CSS
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/CSS/First_steps/Getting_started
    • and 43 more flaws omitted

(comment last updated: 2025-01-09 03:05:23)

estelle
estelle reviewed Jan 6, 2025
View reviewed changes
Copy link
Member

@estelle estelle left a comment

Choose a reason for hiding this comment

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

Instead of describing how to compare aspect ratios in the syntax section, let's put the comparison in an example in the @media page.

files/en-us/web/css/ratio/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/@media/aspect-ratio/index.md Outdated
@@ -11,7 +11,7 @@ The **`aspect-ratio`** [CSS](/en-US/docs/Web/CSS) [media feature](/en-US/docs/We

## Syntax

The `aspect-ratio` feature is specified as a {{cssxref("&lt;ratio&gt;")}} value representing the width-to-height aspect ratio of the viewport. It is a range feature, meaning you can also use the prefixed **`min-aspect-ratio`** and **`max-aspect-ratio`** variants to query minimum and maximum values, respectively.
The `aspect-ratio` feature is specified as a {{cssxref("&lt;ratio&gt;")}} value representing the width-to-height aspect ratio of the viewport. It is a range feature, meaning you can also use the prefixed **`min-aspect-ratio`** and **`max-aspect-ratio`** variants to query minimum and maximum values, respectively. The aspect ratio is [compared using the quotient's numeric value](/en-US/docs/Glossary/Aspect_ratio#comparing_aspect_ratios).
Copy link
Member

Choose a reason for hiding this comment

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

i think the user is looking for an explanation of min- vs. max- aspect ratio not how to compare them. I am not sure this is useful or addresses the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue title confused me. :D

Copy link
Contributor Author

@OnkarRuikar OnkarRuikar Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've updated the whole example:

  • made the intro clearer, including the sample HTML code.
  • added/updated media query comments
  • used uniform colors to avoid confusion: #99f, #9f9, #f99 (blue, green,red)
  • added a border to the iframe to get a better feel of the shape
  • show live aspect ratio so that the viewport's shape and aspect ratio can be related
  • added explanation about max and min media query props

@chrisdavidmills chrisdavidmills removed their request for review January 6, 2025 10:09
@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 Jan 7, 2025
@OnkarRuikar OnkarRuikar requested a review from estelle January 7, 2025 04:29
Josh-Cena
Josh-Cena approved these changes Jan 7, 2025
View reviewed changes
files/en-us/glossary/aspect_ratio/index.md Outdated Show resolved Hide resolved
estelle
estelle reviewed Jan 7, 2025
View reviewed changes
Copy link
Member

@estelle estelle left a comment

Choose a reason for hiding this comment

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

Thanks Onkar. two comments....

files/en-us/glossary/aspect_ratio/index.md Outdated Show resolved Hide resolved
files/en-us/glossary/aspect_ratio/index.md Outdated Show resolved Hide resolved
@OnkarRuikar OnkarRuikar requested a review from estelle January 8, 2025 03:53
@github-actions github-actions bot added size/s [PR only] 6-50 LoC changed and removed size/m [PR only] 51-500 LoC changed labels Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Josh-Cena Josh-Cena Josh-Cena approved these changes

@estelle estelle Awaiting requested review from estelle

Assignees
No one assigned
Labels
Content:CSS Cascading Style Sheets docs Content:Glossary Glossary entries size/s [PR only] 6-50 LoC changed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mention how <ratio> values are compared
3 participants
@OnkarRuikar @estelle @Josh-Cena