content icon indicating copy to clipboard operation
content copied to clipboard

docs(manifest): Improve `icons` member page

Open dipikabh opened this issue 1 year ago • 2 comments

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:

  • https://w3c.github.io/manifest/#icons-member
  • https://w3c.github.io/manifest/#declaring-multiple-icons

Related issues and pull requests

Tracking issue: https://github.com/mdn/mdn/issues/560 display page PR: https://github.com/mdn/content/pull/34386

dipikabh avatar Jul 09 '24 04:07 dipikabh

Preview URLs

External URLs (1)

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

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

github-actions[bot] avatar Jul 09 '24 04:07 github-actions[bot]

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 avatar Aug 20 '24 01:08 dipikabh

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

dipikabh avatar Aug 30 '24 17:08 dipikabh

Thanks, Hamish! This is great feedback and I believe the page is looking much better now. I've addressed your comments, plus made some additional changes:

  • In the "Values" section, I've replaced the property terms with the placeholder values. I've adjusted the introductory prose to reflect this change.
  • Added more info to the <purpose-keywords> description based on the spec. And also clarified the behavior if there are invalid purposes.
  • Added two spec links to "See also"

dipikabh avatar Sep 04 '24 04:09 dipikabh

@dipikabh Aside from https://github.com/mdn/content/pull/34725#discussion_r1751312240 I think this is pretty good. If you compare https://pr34725.content.dev.mdn.mozit.cloud/en-US/docs/Web/Manifest/icons with the original https://developer.mozilla.org/en-US/docs/Web/Manifest/icons you can see that the old way was "prettier" but this contains a lot more information and appears to be a more extensible pattern.

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?

hamishwillee avatar Sep 10 '24 06:09 hamishwillee

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:

  • https://pr34725.content.dev.mdn.mozit.cloud/en-US/docs/Web/Manifest/theme_color
  • https://pr34725.content.dev.mdn.mozit.cloud/en-US/docs/Web/Manifest/display

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

dipikabh avatar Sep 10 '24 22:09 dipikabh

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

hamishwillee avatar Sep 16 '24 01:09 hamishwillee

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.

dipikabh avatar Sep 16 '24 22:09 dipikabh