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

Rewrite attr() to match the newest version of the spec #37574

Merged
merged 54 commits into from
Jan 14, 2025

Conversation

bramus
Copy link
Contributor

@bramus bramus commented Jan 9, 2025

(👋 Hi, Chrome DevRel here)

Description

The attr() function underwent an upgrade: https://drafts.csswg.org/css-values-5/#attr-notation
This PR updates the attr() page with that info.

Motivation

This feature is shipping in the upcoming Chrome 133: https://chromestatus.com/feature/4680129030651904

Additional details

  • This is my first major contribution to MDN, so most likely I got a bunch of things wrong. I modeled the page structure after calc-size()
  • There are still two things marked as TODO: in the file, for which I could use some guidance.

Related issues and pull requests

There is still an issue with the data table, for which I have file mdn/browser-compat-data#25618

@bramus bramus requested a review from a team as a code owner January 9, 2025 16:31
@bramus bramus requested review from estelle and removed request for a team January 9, 2025 16:31
@github-actions github-actions bot added Content:CSS Cascading Style Sheets docs size/m [PR only] 51-500 LoC changed labels Jan 9, 2025
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Preview URLs

Flaws (50)

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

URL: /en-US/docs/Web/CSS/attr
Title: attr()
Flaw count: 50

  • 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 44 more flaws omitted
  • broken_links:
    • Can't resolve /docs/Web/CSS/CSS_Values_and_Units#distance_units
External URLs (3)

URL: /en-US/docs/Web/CSS/attr
Title: attr()


URL: /en-US/docs/Glossary/guaranteed_invalid_value
Title: Guaranteed-invalid value

(comment last updated: 2025-01-14 13:53:20)

@chrisdavidmills chrisdavidmills self-requested a review January 9, 2025 16:37
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.

Hi @bramus! This is a really good effort, thanks for updating this page! I've made quite a few comments, but a lot of them are language-style nitpicks; there isn't too much in terms of significant work to do here.

files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
@chrisdavidmills chrisdavidmills removed the request for review from estelle January 10, 2025 11:59
bramus and others added 19 commits January 10, 2025 15:34
@bramus bramus requested a review from a team as a code owner January 10, 2025 16:05
@bramus bramus requested review from chrisdavidmills and removed request for a team January 10, 2025 16:05
@github-actions github-actions bot added the Content:Glossary Glossary entries label Jan 10, 2025
@bramus
Copy link
Contributor Author

bramus commented Jan 10, 2025

Thanks for the review, Chris. Merged a bunch of your suggestions and addressed the things I could address. Those that I could not address I have left as unresolved + left a comment.

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.

@bramus much improved! A few more comments for you; getting pretty close now.

files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Show resolved Hide resolved
@bramus
Copy link
Contributor Author

bramus commented Jan 13, 2025

Thanks for your input, @chrisdavidmills – Much appreciated! I’ve committed the suggestions you code and am about to go over the remaining conversations now.

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.

@bramus sorry, I forgot to review the new glossary page. One more new comment added!

---

{{GlossarySidebar}}

Copy link
Contributor

Choose a reason for hiding this comment

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

This page reads fine as it is, but I think it is missing one thing — what is the Guaranteed-invalid value in general? What is it's purpose, why do we need it, what does it do? This is not clear after reading this page. The page should start with a sentence "The guaranteed-invalid value is..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does start with that on the line below:

In CSS the guaranteed-invalid value is {{CSSXref("initial")}}.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that says specifically what the value is, but I guess I am still struggling a bit with why this is a specific thing conceptually. Why it is useful to people? Or is it more of a thing to warn people about, not to use this value in var() because it is guaranteed invalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that exceeds the extent of this PR and would be a good subject for a standalone one.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that's fine. I don't think this is something we need to block this PR on.

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.

@bramus really nice work. I've added a few more comments, but I'm going to approve this now, as I think it is basically ready to merge, and my remaining comments are more "nice to have"s.

Have a look and see what you think. I think we can merge this in the next round.

files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Show resolved Hide resolved
files/en-us/web/css/attr/index.md Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/attr/index.md Outdated Show resolved Hide resolved
@bramus
Copy link
Contributor Author

bramus commented Jan 14, 2025

Thanks for your extensive input here @chrisdavidmills! I think I have addressed all remarks.

I suggest to update the new “guaranteed-invalid value” glossary page in a standalone PR.

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.

Awesome work @bramus, this is ready to merge!

I just committed a few small grammar nitpicks directly to the branch, rather than bothering you with multiple tiny comments. I hope that's OK.

@chrisdavidmills chrisdavidmills merged commit 1c93ad1 into mdn:main Jan 14, 2025
9 checks passed
@bramus
Copy link
Contributor Author

bramus commented Jan 14, 2025

I just committed a few small grammar nitpicks directly to the branch, rather than bothering you with multiple tiny comments. I hope that's OK.

Perfect.

Thanks for your thorough review of all this and the successive merge, Chris!

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:Glossary Glossary entries size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants