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(svg attributes): Remove the element list and reorder sections (for attributes u-z ) #32965

Merged
merged 9 commits into from Apr 19, 2024

Conversation

dipikabh
Copy link
Contributor

@dipikabh dipikabh commented Apr 4, 2024

Description

I came across the SVG x attribute page while triaging #32081.

The element list under "You can use this attribute with the following SVG elements:" resembles a Table of Contents (TOC) but it is not. I found this list alongside the "In this article" sidebar links confusing.

The links in the element list take you to the respective element pages whereas I was expecting navigation to sections on the attribute page itself.

Given that the sidebar is meant for in-page navigation, and each element section already includes a link to its corresponding element page, the initial list of links appears to be redundant.

This PR:

  • Removes the element list from attribute pages that contain subsequent sections for those elements. Each section contains a link to the corresponding element page, so that navigation aid is not lost.

  • Renames "Example" to "Examples". This aligns with our use of the plural form for this section. For example, in the SVG element page template, the guideline is: "Note that we use the plural "Examples" even if the page only contains one example."

  • Changes the position of the "Examples" section so that it follows "Usage notes" or appears before "Specifications" (as in SVG element page template and various other templates).
    (The appearance of examples before an explanation of the values was reported as a problem in SVG preserveAspectRatio documentation flow/ordering is confusing #31825.)

  • Uses the section name "Usage notes" when a description of values is included and the name "Usage context" when the section contains only a table. The section name "Context notes" appears only on SVG attribute pages; it has been updated to "Usage notes" or "Usage context" as appropriate.

Note: The improvements in this PR focus on attributes with names starting from U to Z.

Motivation

To help streamline navigation and improve consistency with other parts of MDN

@dipikabh dipikabh requested a review from a team April 4, 2024 19:09
@github-actions github-actions bot added Content:SVG SVG docs size/l 501-1000 LoC changed labels Apr 4, 2024
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.

I made a few suggestions to improve the docs as long as we're in her editing them.

files/en-us/web/svg/attribute/viewbox/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/viewbox/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/width/index.md Show resolved Hide resolved
files/en-us/web/svg/attribute/width/index.md Show resolved Hide resolved
files/en-us/web/svg/attribute/x/index.md Show resolved Hide resolved
files/en-us/web/svg/attribute/x/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/x/index.md Show resolved Hide resolved
Co-authored-by: Estelle Weyl <estelle@openwebdocs.org>
files/en-us/web/svg/attribute/viewbox/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/width/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/word-spacing/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/x/index.md Outdated Show resolved Hide resolved
@dipikabh dipikabh requested a review from a team as a code owner April 9, 2024 19:03
@dipikabh dipikabh requested review from hamishwillee and removed request for a team April 9, 2024 19:03
@dipikabh
Copy link
Contributor Author

dipikabh commented Apr 9, 2024

Hi @estelle, thanks a lot for reviewing these pages!

I've accepted your content additions/improvements (thank you for the suggestions). I believe we can address these concerns in a separate issue/PR. The focus of the current set is to fix the navigation and ordering of sections.

Please see my response in #32965 (comment) for the reasoning behind some of the changes. You can preview them on the viewBox page. If these changes look okay to you, I'll replicate them to other pages. Thanks!

@dipikabh dipikabh requested a review from estelle April 9, 2024 23:30
@hamishwillee hamishwillee removed their request for review April 12, 2024 00:35
@hamishwillee
Copy link
Collaborator

I've removed my review request as it looks like @estelle has sorted this. You can add me back if further review is needed.

@estelle
Copy link
Member

estelle commented Apr 13, 2024

The focus of the current set is to fix the navigation and ordering of sections.

I am not a fan of the h2s as the reader doesn't know if "use" is an element or another section of the page; the elements aren't differentiated from the other page sections. As it's currently like that, and this doesn't address it but doesn't make it worse, no reason to hold up a PR (though opening an issue would be great so we can remember to address this in the future)

side bar bottom where the elements aren 't differentiated from the other page sections.

hmmmmm, should we solve this by adding < and > to the h2 elements?

@dipikabh
Copy link
Contributor Author

dipikabh commented Apr 16, 2024

I am not a fan of the h2s as the reader doesn't know if "use" is an element or another section of the page.

Correct and I agree. I think we're saying the same thing :) Just to be sure:

Current viewBox:

viewbox_original

What I'm proposing (viewBox preview):

viewbox_proposed

I am guessing that the only point where we differ is whether or not to have a hand-curated TOC to various element sub-sections. That is, I am not in favor of including this linked list:

viewbox_toc

@github-actions github-actions bot added size/xl >1000 LoC changed and removed size/l 501-1000 LoC changed labels Apr 16, 2024
@dipikabh
Copy link
Contributor Author

dipikabh commented Apr 16, 2024

Hi @estelle, I've updated the files to remove the elements as H2s and also added code styling to element headers in 110ef88.

As a sample, you can check the width attribute page: https://pr32965.content.dev.mdn.mozit.cloud/en-US/docs/Web/SVG/Attribute/width .

I hope this is looking good to land. Thanks for your review!

@dipikabh dipikabh requested a review from estelle April 16, 2024 15:18
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.

Some of the examples were duplicates, so made suggestions to create different yet useful ones.

files/en-us/web/svg/attribute/vector-effect/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/width/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/word-spacing/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/x1/index.md Show resolved Hide resolved
files/en-us/web/svg/attribute/x2/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/y/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/y/index.md Show resolved Hide resolved
files/en-us/web/svg/attribute/y1/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/y2/index.md Outdated Show resolved Hide resolved
files/en-us/web/svg/attribute/z/index.md Outdated Show resolved Hide resolved
Co-authored-by: Estelle Weyl <estelle@openwebdocs.org>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Estelle Weyl <estelle@openwebdocs.org>
@dipikabh
Copy link
Contributor Author

Thanks for the suggestions, @estelle! I've applied them as is.

It might be better to address the content concerns and updates in a separate PR. This one was specifically aimed at fixing only the navigation, and I was waiting to get this approved before making similar changes to other attribute pages. I hope we can land this soon. In similar subsequent PRs, I'll happy to take up content improvements separately.

@dipikabh dipikabh requested a review from estelle April 19, 2024 15:43
@estelle estelle merged commit 00252e3 into mdn:main Apr 19, 2024
8 checks passed
@dipikabh
Copy link
Contributor Author

Thanks, @estelle 🎉

@dipikabh dipikabh deleted the svg-attributes branch April 22, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:SVG SVG docs size/xl >1000 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants