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.

Sign up for GitHub

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

Jump to bottom

Update flag data for Payment APIs; delete PaymentDetailsUpdate dictionary #12277

Merged
merged 12 commits into from
Dec 2, 2021
Merged

Update flag data for Payment APIs; delete PaymentDetailsUpdate dictionary #12277

merged 12 commits into from
Dec 2, 2021

Conversation

queengooborg
Copy link
Contributor

@queengooborg queengooborg commented Sep 6, 2021 •
edited
Loading

This PR's goal is to delete the PaymentDetailsUpdate dictionary from BCD. First, this copies the Firefox flag, note and version data from PaymentDetailsUpdate to PaymentRequest, PaymentMethodChangeEvent, PaymentRequestUpdateEvent and PaymentResponse, which fixes consistency issues. Afterwards, the entries within the dictionary are copied as subfeatures to both PaymentRequest.show and PaymentRequestUpdateEvent.updateWith, the two methods that use this dictionary. Finally, the dictionary is deleted.

@queengooborg queengooborg added needs-release-note 📰 needs content update This PR needs a corresponding update to mdn/content to update the documentation labels Sep 6, 2021
@github-actions github-actions bot added the data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Sep 6, 2021
@queengooborg queengooborg mentioned this pull request Sep 6, 2021
Merged
@queengooborg queengooborg removed the needs content update This PR needs a corresponding update to mdn/content to update the documentation label Sep 7, 2021
@queengooborg queengooborg requested a review from foolip September 21, 2021 08:59
foolip
foolip previously requested changes Sep 22, 2021
View reviewed changes
@queengooborg queengooborg changed the title Update flag data for PaymentRequest[UpdateEvent]; delete PaymentDetailsUpdate dictionary Update flag data for Payment APIs; delete PaymentDetailsUpdate dictionary Sep 22, 2021
@mdn mdn deleted a comment Sep 23, 2021
foolip
foolip reviewed Sep 27, 2021
View reviewed changes
@foolip
Copy link
Contributor

foolip commented Sep 30, 2021

The PaymentDetailsUpdate dictionary is also used (as optional Promise<PaymentDetailsUpdate> detailsPromise) for PaymentRequest's show() method. If it is worth capturing this data at all, it should be duplicated there. I'm not totally sure that it is however and would like to hear from @ddbeck or @Elchi3 how they'd deal with this situation.

@foolip
Copy link
Contributor

foolip commented Sep 30, 2021

I've filed mdn/content#9362 about a content issue here. For these methods that can take either X or a promise resolving to X I think it makes sense in BCD to just treat it as X, but it is novel and maybe the data guideline needs to be updated for this.

ddbeck
ddbeck requested changes Oct 12, 2021
View reviewed changes
Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Noticed something while I was here to answer another question.

@ddbeck
Copy link
Collaborator

ddbeck commented Oct 12, 2021

@foolip My take is that, if we can avoid it, don't bother with the subfeature. If the method always supported a parameter, then we don't need to break out the parameter specifically (though it's not wrong to include it, I guess). The only oddity I think that would require a new guideline is if promise-resolves-to-X-or-X was supported before/after X (that is, the supported types of the parameter changed). Feels like something that could be split into two support statements (e.g., one partial, one not) or similar. No extra structures needed.

This assumes I understand what's going on here, which maybe I don't!

@foolip
Copy link
Contributor

foolip commented Oct 20, 2021

Thanks @ddbeck! I've compared the data for the removed PaymentDetailsUpdate entry to show() and updateWith() and while it's not exactly the same for Chrome and Edge, it's close enough and far back enough to not be worth capturing. If we dug into it, me might find that it's just BCD that's wrong anyway.

@foolip foolip dismissed their stale review October 20, 2021 08:24

deferring to ddbeck

@foolip
Copy link
Contributor

foolip commented Oct 20, 2021

I was going to give this a final review, but it's all a bit too involved. I'd like someone at Mozilla to vet this, so I've dismissed my own earlier review. @ddbeck can you handle this one?

@ddbeck
Copy link
Collaborator

ddbeck commented Oct 20, 2021

@foolip yes, I'll take on reviewing this—I'm trying to schedule some time daily for BCD reviews, so I hope to get to this sooner rather than later.

ddbeck
ddbeck requested changes Oct 21, 2021
View reviewed changes
Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

A bunch of little suggestions for you, @queengooborg. Thanks!

@ddbeck
Copy link
Collaborator

ddbeck commented Nov 5, 2021

@queengooborg I see that a bunch of the suggestions have resolved, but not others. Anything I can do to help this along? Or is it just a work in progress?

@queengooborg
Copy link
Contributor Author

Sorry, this one slipped my mind! I just applied the suggested changes and I see the two comments regarding the nightly-availability builds.

@queengooborg queengooborg requested a review from ddbeck November 30, 2021 13:31
ddbeck
ddbeck reviewed Nov 30, 2021
View reviewed changes
api/PaymentResponse.json Outdated
"version_added": false,
"notes": "Available only in nightly builds."
"version_added": "55",
"notes": "Available only in nightly builds.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's been a while since I've looked at this, but I think we talked about dropping these notes on the suspicion that they're not accurate (otherwise, we could use "preview" values, if they're true).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! Somehow I didn't catch these notes in the removal, fixed!

ddbeck
ddbeck approved these changes Dec 2, 2021
View reviewed changes
Copy link
Collaborator

@ddbeck ddbeck 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! 🎉

@ddbeck ddbeck merged commit 5866988 into mdn:main Dec 2, 2021
@queengooborg queengooborg deleted the api/PaymentRequest branch December 2, 2021 10:19
jpmedley pushed a commit that referenced this pull request Jan 10, 2022
@queengooborg @ddbeck @jpmedley
Update flag data for Payment APIs; delete PaymentDetailsUpdate dictio…
224e9cb 
…nary (#12277)

* Update flag data for PaymentRequest[UpdateEvent]

* Delete PaymentDetailsUpdate dictionary

* Update PaymentMethodChangeEvent and PaymentResponse

* Remove notes

* Remove subfeatures

* Remove non-needed subfeatures

* Add version_removed for Firefox Android

* Apply suggestions from code review

Co-authored-by: Daniel D. Beck <[email protected]>

* Fix indentation

* Apply suggestions from code review

Co-authored-by: Daniel D. Beck <[email protected]>

* Remove notes

* Remove remaining notes

Co-authored-by: Daniel D. Beck <[email protected]>
@mdn mdn deleted a comment Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foolip foolip foolip left review comments

@ddbeck ddbeck ddbeck approved these changes

Assignees
No one assigned
Labels
data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@queengooborg @foolip @ddbeck