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

feat: Editorial review for Shared Storage API docs #30427

Merged
merged 134 commits into from
Dec 6, 2023

Conversation

chrisdavidmills
Copy link
Contributor

Description

#28051 contains the engineering technical review for my work on the Shared Storage API docs, which has been completed and approved. Thank you to @pythagoraskitty for your thorough and detailed review work.

This is a new PR based on the same branch, which is intended to contain the editorial review for the same work.


Background information

The Shared Storage API is an integral part of Google's privacy sandbox technologies. Many parts of this set are being made available by default in Chrome 117 (depending on a gradual ramp-up to 100% of userbase over the 117 release period).

This PR adds content for Shared Storage. See my research document for information on exactly what features are being added.

Motivation

Additional details

Related issues and pull requests

chrisdavidmills and others added 26 commits July 19, 2023 15:09
@chrisdavidmills chrisdavidmills requested review from a team as code owners November 21, 2023 09:16
@chrisdavidmills chrisdavidmills requested review from sideshowbarker and removed request for a team November 21, 2023 09:16
chrisdavidmills and others added 17 commits November 28, 2023 09:26
@chrisdavidmills
Copy link
Contributor Author

  • All occurrences of this link need to be updated (storage --> Storage)
    [Shared Storage API](/en-US/docs/Web/API/Shared_storage_API)

  • Also, if you're updating key/value pairs, you might want to check all occurrences (key/value pairs --> key-value pairs)

@dipikabh I've done both of these. For the second one, I've made sure the key/value --> key-value change has been made everywhere, but just in the shared storage api docs. Looking at the rest of MDN, it looks like key/value is still used everywhere. Are the changes made in this PR part of a larger effort to standardize on key-value everywhere?

@dipikabh
Copy link
Contributor

dipikabh commented Dec 5, 2023

I've done both of these.

Thank you!

it looks like key/value is still used everywhere. Are the changes made in this PR part of a larger effort to standardize on key-value everywhere?

That's an interesting question. I came across key/value in this PR. My preference leans towards key-value and a quick search indicated that the hyphenated form has been used elsewhere, so I was okay to make the suggestion. It appears that both 'key-value' (88) and 'key/value' (96) are in use across our docs. To the best of my knowledge, there is no ongoing effort to standardize this usage.

I'll take a quick look again at the PR shortly and we should be able to get this one through today

@chrisdavidmills
Copy link
Contributor Author

That's an interesting question. I came across key/value in this PR. My preference leans towards key-value and a quick search indicated that the hyphenated form has been used elsewhere, so I was okay to make the suggestion. It appears that both 'key-value' (88) and 'key/value' (96) are in use across our docs. To the best of my knowledge, there is no ongoing effort to standardize this usage.

Ah, so it is used in other places on the site too! OK, all good then. We should maybe agree on a standard for this one day, but it doesn't have to be today.

I'll take a quick look again at the PR shortly and we should be able to get this one through today

Lovely, cheers!

Copy link
Contributor

@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.

Hi @chrisdavidmills, thanks for the updates and addressing all the suggestions.

I like that you quote your edited text if it is different from the suggestion. It helps me quickly go through any outstanding changes I should look at. I've started following this style in my PRs as well 🙂.

I'm leaving a +1 here, but I'll leave it for you to merge in case you want to update the text for these two comments:
#30427 (comment)
#30427 (comment)

@chrisdavidmills
Copy link
Contributor Author

I like that you quote your edited text if it is different from the suggestion. It helps me quickly go through any outstanding changes I should look at. I've started following this style in my PRs as well 🙂.

@dipikabh I've only recently started doing that as well; pretty useful huh.

I've addressed the further couple of minor changes you suggested, and now I think we're ready to merge. Thanks so much for the detailed review!

@chrisdavidmills chrisdavidmills merged commit d484785 into mdn:main Dec 6, 2023
8 checks passed
@chrisdavidmills chrisdavidmills deleted the shared-storage-api branch December 6, 2023 10:19
@chrisdavidmills chrisdavidmills restored the shared-storage-api branch December 6, 2023 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants