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

29787 text wrap #30435

Merged
merged 35 commits into from
Dec 18, 2023
Merged

29787 text wrap #30435

merged 35 commits into from
Dec 18, 2023

Conversation

dletorey
Copy link
Contributor

Description

  • Added the different values for text-wrap
  • Added text-wrap to experimental features
  • Added text-wrap 121 release note

Motivation

Working on the following Issues:

Additional details

Related issues and pull requests

@dletorey dletorey requested review from a team as code owners November 21, 2023 16:21
@dletorey dletorey requested review from dipikabh and removed request for a team November 21, 2023 16:21
@github-actions github-actions bot added Content:CSS Cascading Style Sheets docs Content:Firefox Content in the Mozilla/Firefox subtree labels Nov 21, 2023
Copy link
Contributor

github-actions bot commented Nov 21, 2023

Preview URLs

External URLs (3)

URL: /en-US/docs/Web/CSS/text-wrap
Title: text-wrap


URL: /en-US/docs/Mozilla/Firefox/Releases/121
Title: Firefox 121 for developers


URL: /en-US/docs/Mozilla/Firefox/Experimental_features
Title: Experimental features in Firefox

(comment last updated: 2023-12-18 15:30:17)

files/en-us/web/css/text-wrap/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/text-wrap/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/text-wrap/index.md Outdated Show resolved Hide resolved
files/en-us/mozilla/firefox/releases/121/index.md Outdated Show resolved Hide resolved
files/en-us/mozilla/firefox/experimental_features/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/text-wrap/index.md Outdated Show resolved Hide resolved
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.

We need to add the spec https://drafts.csswg.org/css-text-4/#white-space-property to the white-space page, as this property is part of that now shorthand value. We shouldn't remove css-text-3, though, until the shorthand syntax / values are supported.

I think the newer version you wrote was much easier for non-native speakers, but I still wasn't 100% in love with the section under description. I suggested an alternative for that entire section.

files/en-us/web/css/text-wrap/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/text-wrap/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/text-wrap/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/text-wrap/index.md Show resolved Hide resolved
Copy link
Member

@pepelsbey pepelsbey left a comment

Choose a reason for hiding this comment

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

Thank you! Merging it for tomorrow’s release. We can address any questions that might still left in the next PRs.

@pepelsbey pepelsbey removed the request for review from estelle December 18, 2023 15:46
@pepelsbey pepelsbey dismissed estelle’s stale review December 18, 2023 15:48

All the requests are seems to be resolved

@pepelsbey pepelsbey merged commit 3296dca into mdn:main Dec 18, 2023
7 checks passed
@estelle
Copy link
Member

estelle commented Dec 18, 2023

@pepelsbey Do not dismiss reviews, especially without any attempts to reach out to request a re-review.
Requesting changes: use the request changes option when the feedback you provided needs to be addressed by the author and re-reviewed byt the reviewer before the pull request can be approved and merged.

@pepelsbey
Copy link
Member

@estelle I'm sorry but we needed this PR merged before tomorrow's Firefox 121 release. I asked @dletorey if your feedback was fully implemented and made the call. I hope you can still follow up on any potential issues left in a separate issue or PR. Thank you!

@Josh-Cena
Copy link
Member

@dletorey @pepelsbey @zfox23 and other Mozilla folks: to make sure this doesn't happen again, please split your content PR and FF release note PR. The non-moz folks would likely not block your release notes so you can chill a bit about content. I know Hamish always does this.

@pepelsbey
Copy link
Member

please split your content PR and FF release note PR

Yes, I fully agree. But it won't solve the problem, unfortunately: when some features get released in Firefox, we update not only release notes but also content. We were hoping a few weeks would be enough to get this review done. It wasn't, so I had to merge it.

I think we might also consider an approach where we can merge less than ideal pages first and then start improving them later. Otherwise, it stops developers from getting this information from MDN. The new text-wrap values were implemented in other browsers a while ago.

@Josh-Cena
Copy link
Member

Josh-Cena commented Dec 18, 2023

Any content that appears to users should be well-reviewed and agreed upon by parties involved. Reviewers not involved implicitly approve, but reviewers who explicitly disprove need to be given the chance for another round of review. This is one of the core reasons why we moved away from wiki: so we never have subpar pages getting into production :)

However, I agree we are missing an explicit timebox here. We need contributing guidelines for how long to wait for reviews.

@dletorey dletorey mentioned this pull request Dec 19, 2023
dipikabh pushed a commit to dipikabh/content that referenced this pull request Jan 17, 2024
* added pretty & stable to text wrap

* added a section on when balance/pretty/stable should be used

* Updated the heading to Description

* added the experimental release note for text-wrap

* added text-wrap to the release note 121

* Update files/en-us/web/css/text-wrap/index.md

Co-authored-by: Zach Fox <[email protected]>

* Update files/en-us/web/css/text-wrap/index.md

Co-authored-by: Estelle Weyl <[email protected]>

* Update files/en-us/mozilla/firefox/releases/121/index.md

Co-authored-by: Zach Fox <[email protected]>

* Update files/en-us/mozilla/firefox/experimental_features/index.md

Co-authored-by: Zach Fox <[email protected]>

* Update files/en-us/web/css/text-wrap/index.md

Co-authored-by: Zach Fox <[email protected]>

* simplified the language speed over paint performance

* Update files/en-us/web/css/text-wrap/index.md

Co-authored-by: Estelle Weyl <[email protected]>

* made the description of stable clearer

* Update files/en-us/mozilla/firefox/releases/121/index.md

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

* Update files/en-us/web/css/text-wrap/index.md

Co-authored-by: Estelle Weyl <[email protected]>

* Update files/en-us/web/css/text-wrap/index.md

Co-authored-by: Estelle Weyl <[email protected]>

* Update files/en-us/web/css/text-wrap/index.md

Co-authored-by: Estelle Weyl <[email protected]>

* working on Description

* updated the description to be simpler

* added heading as an example

* fixed the flaw in the headings link

---------

Co-authored-by: Zach Fox <[email protected]>
Co-authored-by: Estelle Weyl <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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 Content:Firefox Content in the Mozilla/Firefox subtree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants