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

Update complete() method to take a string and clarify its purpose #161

Closed
wants to merge 2 commits into from

Conversation

adrianba
Copy link
Contributor

@adrianba adrianba commented Apr 27, 2016

The argument to complete is used to influence the user experience as the
user interface for the payment request is dismissed. Some sites will not
be able to provide an indication of success (perhaps because they
require additional flow steps after the payment request before accepting
an order). Calling complete() with no arguments uses the default value
where success or failure are not indicated.

The argument to complete is used to influence the user experience as the
user interface for the payment request is dismissed. Some sites will not
be able to provide an indication of success (perhaps because they
require additional flow steps after the payment request before accepting
an order). Calling complete() with no arguments uses the default value
where success or failure are not indicated.

NOTE: the current WebIDL parser used by Respec does not handle default
values for enums though these are allowed in WebIDL. This results in a
Respect error once this change is merged.
@halindrome
Copy link
Contributor

Have you filed a ReSpec bug on this?

@adrianba
Copy link
Contributor Author

Have you filed a ReSpec bug on this?

This was my mistake. Fixed now.

@msporny
Copy link
Member

msporny commented Apr 28, 2016

+1 to merge, this is better than the boolean in the spec right now. I feel a bit uneasy about the string value of "" (and the list of potential values growing in the future) and how a browser would affect the UI flow based on that value, but expect the browser vendors to give proper feedback on this in time. From a developers perspective, what you pass to the function is more clear now by just reading the code... I know what .complete("success") and .complete("fail") mean.

What happens when someone does .complete("failure")?

@dlongley
Copy link

I prefer .succeed() and .fail() or something along those lines. Passing a string isn't all that much better than a boolean (which is bad), IMO. Could consider passing a Promise to be resolved/rejected.

@msporny
Copy link
Member

msporny commented Apr 28, 2016

To be clear, I prefer .succeed() and .fail() too, but baby steps toward that. I'd rather have this PR in the spec than what's in the spec right now.

@adrianhopebailie
Copy link
Collaborator

LGTM

@adrianba
Copy link
Contributor Author

@msporny wrote:

I feel a bit uneasy about the string value of ""

Yeah, I'm not totally wedded to this - the reason I chose "" is because it is falsy in JavaScript.

@adrianba
Copy link
Contributor Author

adrianba commented Apr 28, 2016

@dlongley wrote:

I prefer .succeed() and .fail() or something along those lines.

Would those methods take arguments? What if you don't have a success or a failure - you just don't know yet. Would you propose a third method? If we have three methods then at some point we might have four or more methods?

@adrianba
Copy link
Contributor Author

@msporny wrote:

and the list of potential values growing in the future

Good point. There is a downside in that adding new values doesn't indicate whether they are successful values or failing values. For example, what if we wanted a new UX for "timeout" that was distinct from a general failure animation.

@dlongley
Copy link

dlongley commented Apr 28, 2016

Could do: complete({status: <enum>}); like complete({status: 'success'});

If we want to support the most extensibility.

@jnormore
Copy link
Member

👍 to extensibility with status @dlongley

My thoughts behind this are that eventually we could be convinced to add more error handling functionality (like an error message supplied by the merchant).

@adrianba
Copy link
Contributor Author

Merged as 5bcadcc per telcon.

@adrianba adrianba closed this Apr 28, 2016
@adrianba adrianba deleted the issue17-complete branch April 28, 2016 17:13
adrianba added a commit to adrianba/browser-payment-api that referenced this pull request Apr 28, 2016
PRs w3c#158, w3c#160, and w3c#161 made changes that affect the examples. These
weren't reflected in original PRs to keep those changes simple and to
avoid merge conflicts from changing the same lines in the spec. This
changes updates the examples.

Also updating the syntax to be consistent and use JavaScript (rather
than JSON).

Fixes w3c#164.
adrianba added a commit that referenced this pull request Apr 28, 2016
PRs #158, #160, and #161 made changes that affect the examples. These
weren't reflected in original PRs to keep those changes simple and to
avoid merge conflicts from changing the same lines in the spec. This
changes updates the examples.

Also updating the syntax to be consistent and use JavaScript (rather
than JSON).

Fixes #164.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants