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

docs(manifest): Improve icons member page #34725

Merged
merged 43 commits into from
Sep 16, 2024
Merged

Conversation

dipikabh
Copy link
Contributor

@dipikabh dipikabh commented Jul 9, 2024

Description

This work is part of improving the web/manifest docs. This PR focuses on the icons member page.

Some notable changes in this PR:

  • Removed the "Type" table from the top of the page. The type can be covered in the Syntax section and does not need a table. (On some other pages like file_handlers, this Type table overcrowds with technology status banners.)
  • Created a "Syntax" section with "Values" and "Properties" subsections. Replaced the table describing the values with a definition list format.
  • Created a "Description" section to include details about security and performance aspects.
  • Added an explanation for the example.

Motivation

To better explain the properties and example and make the page compliant with our page templates

Additional details

Spec links:

Related issues and pull requests

Tracking issue: mdn/mdn#560
display page PR: #34386

@dipikabh dipikabh requested a review from a team as a code owner July 9, 2024 04:54
@dipikabh dipikabh requested review from hamishwillee and removed request for a team July 9, 2024 04:54
@github-actions github-actions bot added Content:Manifest size/m [PR only] 51-500 LoC changed labels Jul 9, 2024
Copy link
Contributor

github-actions bot commented Jul 9, 2024

Preview URLs

External URLs (1)

URL: /en-US/docs/Web/Manifest/icons
Title: icons

(comment last updated: 2024-09-16 22:08:34)

@dipikabh dipikabh changed the title docs(manifest): Imporve icons member page docs(manifest): Improve icons member page Jul 9, 2024
@Josh-Cena Josh-Cena added the awaiting response Awaiting for author to address review/feedback label Aug 16, 2024
@dipikabh
Copy link
Contributor Author

Hi @hamishwillee, thanks a lot for the review and valuable inputs.

Sorry, I was able to get back to this PR only now so you might need to refresh your memory again on this. I've addressed all your comments and indicated parts that are slightly different from the suggestions. For sizes, I've proposed an updated version. Could you take another look when you have a moment? Thanks!

@dipikabh dipikabh removed the awaiting response Awaiting for author to address review/feedback label Aug 20, 2024
files/en-us/web/manifest/icons/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/icons/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/icons/index.md Outdated Show resolved Hide resolved
@dipikabh
Copy link
Contributor Author

Just a nudge to bring this back on your reviewing radar when you get the chance :). Thanks!

@dipikabh
Copy link
Contributor Author

That said, we should update at least one of your other examples with just key-value to confirm the pattern works before merging this. Seem reasonable?

Hi @hamishwillee, I've applied the formatting to two other members:

If it makes it simpler, I can gt rid of the Syntax section (and upgrade Keys to H2).

hamishwillee and others added 3 commits September 13, 2024 10:03
@hamishwillee
Copy link
Collaborator

New structure looks good to me. I added a few comments/suggestions. The most important is https://github.com/mdn/content/pull/34725/files#r1759389846

@dipikabh
Copy link
Contributor Author

Thanks, Hamish, for your time and patience in ironing out the Syntax block format and brainstorming all the Values/Keys/Properties shenanigans.

Appreciate you taking another close look at the rest of the content. The latest set of updates are in.

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

@dipikabh Thanks for this - looks good to me - worth getting this in, fixing up all the rest to match, then we can iterate.

It is a lot easier to be critical than to generate new content and structures in the first place :-)

@hamishwillee hamishwillee merged commit 7742fc9 into mdn:main Sep 16, 2024
8 checks passed
@dipikabh dipikabh deleted the manifest-icons branch September 17, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Manifest size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants