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

Add spec URLs for CSS Flexbox #7161

Merged
merged 10 commits into from
Feb 24, 2021
Merged

Add spec URLs for CSS Flexbox #7161

merged 10 commits into from
Feb 24, 2021

Conversation

foolip
Copy link
Collaborator

@foolip foolip commented Oct 28, 2020

Extracted from w3c@4f66425

@foolip
Copy link
Collaborator Author

foolip commented Oct 28, 2020

I created this to do self-review of some changes and help answer #6765 (comment).

@github-actions github-actions bot added the data:css 🎨 Compat data for CSS features. https://developer.mozilla.org/docs/Web/CSS label Oct 28, 2020
css/properties/align-content.json Outdated Show resolved Hide resolved
css/properties/align-content.json Outdated Show resolved Hide resolved
css/properties/align-items.json Outdated Show resolved Hide resolved
css/properties/align-items.json Outdated Show resolved Hide resolved
css/properties/align-self.json Outdated Show resolved Hide resolved
css/properties/justify-content.json Outdated Show resolved Hide resolved
css/properties/margin-left.json Outdated Show resolved Hide resolved
css/properties/min-width.json Outdated Show resolved Hide resolved
css/properties/visibility.json Outdated Show resolved Hide resolved
css/types/flex.json Outdated Show resolved Hide resolved
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

As said previously, I don't think we should link to snapshots / leveled specs. More suggestions inline.

css/properties/align-content.json Outdated Show resolved Hide resolved
css/properties/align-content.json Outdated Show resolved Hide resolved
css/properties/align-items.json Outdated Show resolved Hide resolved
css/properties/align-self.json Outdated Show resolved Hide resolved
css/properties/display.json Outdated Show resolved Hide resolved
css/properties/margin-left.json Outdated Show resolved Hide resolved
css/properties/margin-right.json Outdated Show resolved Hide resolved
css/properties/min-width.json Outdated Show resolved Hide resolved
css/properties/order.json Outdated Show resolved Hide resolved
css/properties/visibility.json Outdated Show resolved Hide resolved
@foolip foolip marked this pull request as ready for review February 19, 2021 09:36
@Elchi3
Copy link
Member

Elchi3 commented Feb 22, 2021

An overall question in this PR seems to be:

Should we link to sections or directly to the definitions? There are suggestions going on both directions now.

Definition: https://drafts.csswg.org/css-box/#propdef-margin-left
Section: https://drafts.csswg.org/css-box/#margin-physical

@sideshowbarker for spec tooling, which one is better?

@sideshowbarker
Copy link
Collaborator

An overall question in this PR seems to be:

Should we link to sections or directly to the definitions? There are suggestions going on both directions now.

Definition: drafts.csswg.org/css-box/#propdef-margin-left
Section: drafts.csswg.org/css-box/#margin-physical

@sideshowbarker for spec tooling, which one is better?

I guess the question is, which provides a better user experience for web developers? When web developers are reading the spec, is it a better user experience for them to find MDN/BCD annos next to the section headings, or further down next to where the definitions are in the prose?

Personally, having the section headings be where the developers find the MDN/BCD annos seems like better user experience in the general case. However, it can been poorer user experience if we have multiple annos associated with a particular leading. But if it’s only single anno, then it seems better next to heading.

But that’s just my own subjective perception of it. The spec tooling itself is otherwise agnostic to it.

@Elchi3
Copy link
Member

Elchi3 commented Feb 22, 2021

Okay, I think you have more experience with browsing specs and the user experience thereof.

If from a spec tooling and data perspective, both, sections and definitions are equal, then I would say let's take the user experience aspect into account and link to sections instead of definitions.

Other opinions? @foolip?

@foolip
Copy link
Collaborator Author

foolip commented Feb 22, 2021

I prefer linking to sections myself since it will then also include any introductory bits before the definition.

There are cases where this won't work out great, such as when linking the members of IDL interfaces. If we link them all to the section which defines them, it's going to look repetitive and redundant.

But perhaps we can prefer sections for now and figure out that later.

@Elchi3
Copy link
Member

Elchi3 commented Feb 22, 2021

There will likely be exceptions but lets prefer sections when possible then.

Can you make changes throughout this PR and then re-request my review?

css/properties/display.json Outdated Show resolved Hide resolved
@foolip
Copy link
Collaborator Author

foolip commented Feb 22, 2021

@Elchi3 I'm finished updating things now :)

@foolip foolip requested a review from Elchi3 February 22, 2021 20:35
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Just one nit per the CI failure. Happy to merge otherwise.

css/properties/align-items.json Outdated Show resolved Hide resolved
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thanks for sorting this all out, Philip and Mike 🎉

@Elchi3 Elchi3 merged commit 2ec4811 into mdn:master Feb 24, 2021
@foolip foolip deleted the flexbox-spec-urls branch February 24, 2021 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:css 🎨 Compat data for CSS features. https://developer.mozilla.org/docs/Web/CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants