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

flex-basis: 0/0%: Fix: Issue #37597 #37601

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

flex-basis: 0/0%: Fix: Issue #37597 #37601

wants to merge 2 commits into from
+2 −2

Conversation

SurajKharkwal
Copy link

Description

This pull request corrects a typo in the flex-basis documentation under the section "Flex basis: 0 vs 0%".

Related Issues and Pull Requests

Fixes #37597
Relates to the typo in the Mdn documentation here.

@SurajKharkwal SurajKharkwal requested a review from a team as a code owner January 11, 2025 07:48
@SurajKharkwal SurajKharkwal requested review from estelle and removed request for a team January 11, 2025 07:48
@github-actions github-actions bot added Content:CSS Cascading Style Sheets docs size/xs [PR only] 0-5 LoC changed labels Jan 11, 2025
@SurajKharkwal SurajKharkwal mentioned this pull request Jan 11, 2025
Open
@estelle estelle changed the title Fix: Issue #37597 flex-basis: 0/0%: Fix: Issue #37597 Jan 11, 2025
@SurajKharkwal
Copy link
Author

@Hexstream I've addressed the feedback you provided and made the requested updates. Let me know if there's anything else you'd like me to adjust. This is my first time contributing, so if you have any suggestions for improvement, I'd really appreciate them. Thanks again for reviewing!

@Hexstream
Copy link

Awesome! 👍

Could you rebase your 2 commits into 1?

@SurajKharkwal
Copy link
Author

Done

@SurajKharkwal SurajKharkwal reopened this Jan 12, 2025
@github-actions GitHub Actions
Copy link
Contributor

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

Preview URLs

Flaws (48)

URL: /en-US/docs/Web/CSS/flex-basis
Title: flex-basis
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-13 05:21:04)

Hexstream
Hexstream reviewed Jan 12, 2025
View reviewed changes
files/en-us/web/css/flex-basis/index.md Outdated
@@ -176,7 +176,7 @@ This example demonstrates the difference between a `flex-basis` of `0` versus a

#### HTML

We include two same-structure flex containers. These which will be styled similarly, except for their `flex-basis` values. The containers each have two children: a heading `<div>` and a `<section>`. The `<section>` element has a content `<div>` child, which will not be set as a flex item but will be given a height.
We include two same-structure flex containers, which will be styled similarly, except for their `flex-basis` values. The containers each have two children: a heading `<div>` and a `<section>`. The `<section>` element has a content `<div>` child, which will not be set as a flex item but will be given a height.
Copy link

@Hexstream Hexstream Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would remove the comma after "similarly", as initially suggested, otherwise perfect!

@SurajKharkwal
Copy link
Author

SurajKharkwal commented Jan 13, 2025 •
edited
Loading

@estelle , @Hexstream , Let me know if anything else is required

@Hexstream
Copy link

Hexstream commented Jan 13, 2025 •
edited
Loading

So, this faithfully implements my suggestions, and I see that you have slipped in 2 other changes. ;)

You changed:

height 300px

to:

a height of 300px

I like that! 👍

As for the added comma right before it, I don't think it's really better than without it,
I think it just slows down the sentence unnecessarily, but I'm not hell-bent against it.

@SurajKharkwal
Copy link
Author

You need

but the heights of section elements cannot exceed 200px, and their children have a height of 300px.

to

but the heights of section elements cannot exceed 200px and their children have a height of 300px.

right?

@SurajKharkwal
Fix: Issue mdn#37597
975bfad 
Fix: mdn#37597

Fix: Issue mdn#37597

Fix: Issue mdn#37597
@SurajKharkwal
Copy link
Author

Done , Sorry for my mistakes

@Hexstream
Copy link

Sorry, could you rebase your commits into 1 again? 😅

And the commit message on one of your commits is weirdly repetitive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estelle estelle estelle left review comments

@Hexstream Hexstream Hexstream left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Content:CSS Cascading Style Sheets docs size/xs [PR only] 0-5 LoC changed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Slightly awkward sentences on flex-basis page
3 participants
@SurajKharkwal @Hexstream @estelle