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): Fix flex: 1 explanation #37553

Merged
merged 4 commits into from
Jan 14, 2025
Merged

Conversation

OnkarRuikar
Copy link
Contributor

The current statement is correct if you directly see the end result. However, as the sentence uses flex-basis: 0, we need to be more specific about what actually happens behind the scenes.

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner January 8, 2025 04:35
@OnkarRuikar OnkarRuikar requested review from chrisdavidmills and removed request for a team January 8, 2025 04:35
@github-actions github-actions bot added Content:CSS Cascading Style Sheets docs size/xs [PR only] 0-5 LoC changed labels Jan 8, 2025
…flexbox/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

github-actions bot commented Jan 8, 2025

Preview URLs

Flaws (48)

URL: /en-US/docs/Web/CSS/CSS_flexible_box_layout/Basic_concepts_of_flexbox
Title: Basic concepts of flexbox
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-14 01:41:46)

@Hexstream
Copy link

Could you rebase these 3 commits into 1?

@OnkarRuikar
Copy link
Contributor Author

Could you rebase these 3 commits into 1?

Why? The Files changed tab shows combined changes. And while merging, all the commits will get squashed into one.

@Hexstream
Copy link

Hexstream commented Jan 13, 2025

Why?

Because it's almost always better to simplify as early as possible in any pipeline.
I strongly suggest adopting this as a systematic habit, because it promotes efficiency and correctness.

The Files changed tab shows combined changes.

I noticed, but the Commits tab does not, for instance.
One commit per semantic change is much cleaner.

And while merging, all the commits will get squashed into one.

Do you know that, or are you assuming that?

I am not familiar enough with this particular pipeline to know if that's the case or not, but getting into the habit of squashing properly is certainly advised, as that is always a great initiative regardless of pipeline, and it helps you be better organized, too.

Just because some tools compensate for infelicities, does not necessarily mean you should not solve problems at the source before they potentially create more problems.

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

@OnkarRuikar sounds basically good to me. I think the "items start" change is needed for consistency, but I also suggested a new bit to clarify what you mean. What do you think?

I don't feel that strongly about it; Approving.

…flexbox/index.md

Co-authored-by: Chris Mills <chrisdavidmills@gmail.com>
@Josh-Cena
Copy link
Member

@Hexstream: quite the contrary. You will find many open source projects (at least, many that I work on) strongly advise against single-commit PRs.

  1. Commits are meant to show atomic pieces of work. For a PR that does multiple things at once (e.g. refactor, then new feature, then tests), multiple commits help reviewers put things into context and more easily see what changed at each step.
  2. GitHub does a really bad job at generating notifications when you force push. Some projects I've been on go as far as forbidding force pushing (for non-main branches) altogether: no squashing, no rebasing. Because otherwise on GitHub you get a notification but when you click into it it tells you "commit not found".
  3. Many prominent open source projects, including MDN, enforce squash merging to preserve linear history. So yes, to answer your question: PR commit history is irrelevant, and we appreciate contributors to not manually squash merge because multiple force pushes generates an uglier history in the GH interface (it doesn't say what you did, it just shows many force pushes from commit hash A to commit hash B).

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Perfect, thanks @OnkarRuikar!

@chrisdavidmills chrisdavidmills merged commit bdecbff into mdn:main Jan 14, 2025
9 checks passed
@Hexstream
Copy link

@Josh-Cena Thank you, interesting perspective!

  1. Commits are meant to show atomic pieces of work.

Well, that's what I said, "One commit per semantic change"...

I still think all commits here really represented one semantic change, an atomic piece of work...

For a PR that does multiple things at once (e.g. refactor, then new feature, then tests), multiple commits help reviewers put things into context and more easily see what changed at each step.

Obviously, although a new feature and its tests should preferably go in the same commit, I think.

  1. [...]

Right, I guess a possible workaround would be to advise people to comment when they force-push...

It would be nice if GitHub would just add better support for this...

multiple force pushes generates an uglier history in the GH interface (it doesn't say what you did, it just shows many force pushes from commit hash A to commit hash B).

Actually, there is a "Compare" feature for seeing the diff between before and after a force-push, isn't it?


Anyway, even based on all this, I still strongly prefer rebased commits for my own projects, but nice to know that may not be the best policy for all projects...

@OnkarRuikar OnkarRuikar deleted the patch-3 branch January 15, 2025 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/xs [PR only] 0-5 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants