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(manifest): Update manifest member pages to follow the same format #35557

Merged
merged 9 commits into from
Sep 24, 2024

Conversation

dipikabh
Copy link
Contributor

Description

Issue: At the moment, the manifest member pages lack consistency: some list values in a table, while in some others, the "Values" section appears after "Examples" or uses a longer section name like "launch_handler item values".

Goal: Before updating the content of individual manifest member pages, I want to align them all so that they have the same sections in the same order and follow the same format.

Changes in this PR:

  • Removed the "Type" table from the top of the page (this info will get covered in the "Values" section).
  • Added a "Syntax" section to all pages.
  • Replaced the table formats in the "Values" section with a definition list format, where applicable.
  • Renamed sections where needed to ensure that they are called "Values" and "Examples".
  • Moved the "Values" section to the correct place on the page, before "Examples", where needed.

Next steps: I haven't made any content updates yet (except for adding the Syntax section), so some pages do not have a "Values" section at this point. I'll tackle this in my next iteration.

Motivation

To standardize the structure and format across manifest member pages

Related issues and pull requests

@dipikabh dipikabh requested a review from a team as a code owner August 22, 2024 19:53
@dipikabh dipikabh requested review from pepelsbey and removed request for a team August 22, 2024 19:53
@github-actions github-actions bot added Content:Manifest size/l [PR only] 501-1000 LoC changed labels Aug 22, 2024
Copy link
Contributor

github-actions bot commented Aug 22, 2024

Preview URLs (14 pages)
Flaws (2)

Note! 12 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/Manifest/categories
Title: categories
Flaw count: 1

  • bad_bcd_queries:
    • No BCD data for query: html.manifest.categories

URL: /en-US/docs/Web/Manifest/screenshots
Title: screenshots
Flaw count: 1

  • bad_bcd_queries:
    • No BCD data for query: html.manifest.screenshots
External URLs (1)

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

(comment last updated: 2024-09-17 18:21:39)

Copy link
Member

@pepelsbey pepelsbey left a comment

Choose a reason for hiding this comment

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

It looks very nice! Much more consistent and readable now 😍

I have a small concern, though.

```json
"screenshots": [
  {
    "form_factor": "narrow" | "wide"
  }
]
```

Presenting values in the code examples separated by pipes looks clear if you know how JSON works, how strict it is, etc. But when you’re learning about manifest for the first time, you might just copy-paste the sample and have a hard time figuring out why your manifest doesn’t work. Browsers won’t show you any warnings if your manifest is malformed.

I searched for true | false kind of cases, and they’re not very common, apart from XSL docs, for example. We can fix this, too. I’ll bring this up on the next content sync. Or you can do it while I’m away :)

I wonder if it would be better to show some sort of obviously non-fitting placeholder with the value type and list values below? These are pretty common on MDN. Something like:

```json
"screenshots": [
  {
    "form_factor": <string>
  }
]
```

- `"narrow"` some desc
- `"wide"` some desc

@dipikabh
Copy link
Contributor Author

dipikabh commented Aug 27, 2024

Thanks for reviewing, @pepelsbey!

You've highlighted how we approach Syntax sections across different technology areas on MDN. :)

Some background on how I went about the syntax on these pages: I wanted to add a "Syntax" section that provides a quick, sort of a scannable, template that lists all available values or property-value pairs, including any keyword values, so readers can get an overview at a glance. For style, I aligned it with the JS and API areas, using camelCase variables in the syntax, rather than following the approach in the CSS area of using examples to demonstrate the syntax.

I see your point about the potential confusion from using the | notation. It's a really good and nuanced observation!
You're right, it might lead to unexpected outcomes if copied and used directly. Even though our guideline says that "Syntax sections are not meant to show runnable code.", I would lean towards reducing the confusion if possible by avoiding the |. And as you've pointed, it's true that this is not a common practice in our documentation for writing syntax.
(I'm probably trying to cram too much information by spelling out the keywords in the syntax. It would have been easier if I could add comments to the JSON 🙂.)

If we skip the keywords in the syntax, we could use the variable name to suggest that specific keywords are expected as values for the said member/property. Combining this idea with your suggestion, we could use:

```json
"screenshots": [
  {
 
    "form_factor": "keywordValue",
    "platform": "keywordValue"
  
  }
]
```

and booleanValue instead of true | false. The subsequent "Values" section will list and explain the possible keyword values in detail. What do you think about this approach?

I'd considered using placeholders like "<image>", "<urlString>", etc. for values, but again, these kind of uses in Syntax in our docs are rare (as in If-None-Match: "<etag_value>").

Update: In CSS, we do use the < > nomenclature to denote date types like <color> as values for properties.

Another update: Resorting to use the < > style with hyphens, as discussed in #35643 (comment). This is basically inline with your initial suggestion 🙂, just not indicating the type.

Copy link
Contributor Author

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

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

@pepelsbey, I've suggested these changes to remove the pipe notation and streamline some other variable names.

files/en-us/web/manifest/display/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/orientation/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/related_applications/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/screenshots/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/serviceworker/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/share_target/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/shortcuts/index.md Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Sep 3, 2024

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Sep 3, 2024
files/en-us/web/manifest/background_color/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/id/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/categories/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/display_override/index.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

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

Update all placeholder values to follow the same format

files/en-us/web/manifest/file_handlers/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/launch_handler/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/name/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/protocol_handlers/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/related_applications/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/serviceworker/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/share_target/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/short_name/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/shortcuts/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/start_url/index.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

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

remove invalid JSON

files/en-us/web/manifest/categories/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/display_override/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/file_handlers/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/launch_handler/index.md Outdated Show resolved Hide resolved
files/en-us/web/manifest/serviceworker/index.md Outdated Show resolved Hide resolved
@dipikabh
Copy link
Contributor Author

dipikabh commented Sep 4, 2024

Hi Vadim, this is ready for you to take another look. Thanks!

@dipikabh dipikabh marked this pull request as draft September 10, 2024 20:26
@dipikabh
Copy link
Contributor Author

Moving this to draft until we decide on a format in #34725

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Sep 16, 2024
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Sep 17, 2024
@github-actions github-actions bot added size/m [PR only] 51-500 LoC changed and removed size/l [PR only] 501-1000 LoC changed labels Sep 17, 2024
@dipikabh
Copy link
Contributor Author

Hi @pepelsbey, we reached a consensus in #34725 (comment) about the format of "Syntax" blocks. It will contain use cases, similar to the "Syntax" blocks in CSS properties, not placeholder variables. icons and theme_color manifest pages just landed with these changes to the "Syntax".

What you need to know for reviewing this PR:

  • Since adding examples to the "Syntax" block will be a bit more involved now, I've subtracted "Syntax" blocks from this PR.
  • I've also removed the files name, short_name, start_url (35847), related_applications, and prefer_related_applications (35906) from this PR because they're part of other ongoing PRs.

@dipikabh dipikabh marked this pull request as ready for review September 17, 2024 18:39
Copy link
Member

@pepelsbey pepelsbey left a comment

Choose a reason for hiding this comment

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

Looks good :) Thank you!

@dipikabh
Copy link
Contributor Author

Thanks for reviewing, Vadim!

@dipikabh dipikabh merged commit 54dbdfc into mdn:main Sep 24, 2024
8 checks passed
@dipikabh dipikabh deleted the manifest-sync-template branch September 24, 2024 13:20
OnkarRuikar pushed a commit to OnkarRuikar/content that referenced this pull request Sep 28, 2024
…mdn#35557)

* update to follow the same template

* update syntax

* update placeholder values

* update placeholder values

* update placeholder values

* remove syntax blocks

* revert changes to files in other PRs
fiji-flo pushed a commit that referenced this pull request Oct 2, 2024
…#35557)

* update to follow the same template

* update syntax

* update placeholder values

* update placeholder values

* update placeholder values

* remove syntax blocks

* revert changes to files in other PRs
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.

2 participants