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

Various improvements to conditionalCreate/Mediation discoverability and uniformity #2046

Merged
merged 11 commits into from
Jul 17, 2024
Merged

Various improvements to conditionalCreate/Mediation discoverability and uniformity #2046

merged 11 commits into from
Jul 17, 2024

Conversation

emlun
Copy link
Member

@emlun emlun commented Mar 19, 2024 •
edited by pr-preview bot
Loading

Various opportunities for improvement I noticed while reviewing PR #1951, but some of this is beyond the scope of that PR. We should merge #1951 first, then this PR scope will be much smaller as most of these commits will have been merged already with #1951.

  • Reword "This operation" to "Any operation" in get() too, for uniformity with the new language in create().
  • Rephrase and shorten recommendation to check isConditionalMediationAvailable() or getClientCapabilities()
  • Add cross-links between conditionalCreate and conditionalMediation conditionalGet client capabilities and where they are used
  • Point out that conditionalMediation conditionalGet and isConditionalMediationAvailable() are equivalent

Preview | Diff

@emlun emlun added type:editorial stat:Blocked Prerequisites are not yet satisfied labels Mar 19, 2024
@emlun
Copy link
Member Author

emlun commented Mar 19, 2024

Current diff compared to PR #1951: https://github.com/w3c/webauthn/pull/2046/files/aca236f39be577276a7ff962badc33ad6f0d41fb..1df325885582da74f580a4276fc171a707fc655d

@pascoej pascoej self-requested a review March 20, 2024 19:47
@nsatragno nsatragno self-requested a review March 27, 2024 18:05
timcappalli
timcappalli reviewed Apr 23, 2024
View reviewed changes
index.bs Outdated Show resolved Hide resolved
@akshayku akshayku self-requested a review May 15, 2024 19:28
@emlun emlun marked this pull request as ready for review May 30, 2024 13:39
@emlun emlun requested a review from timcappalli May 30, 2024 13:39
@emlun emlun removed the stat:Blocked Prerequisites are not yet satisfied label May 30, 2024
timcappalli
timcappalli approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@timcappalli timcappalli left a comment

Choose a reason for hiding this comment

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

LGTM

MasterKale
MasterKale approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@MasterKale MasterKale left a comment

Choose a reason for hiding this comment

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

Just one minor nitpick, otherwise this looks good to me.

index.bs Outdated Show resolved Hide resolved
@emlun @MasterKale
Fix review comment
0ba20ab 
Co-authored-by: Matthew Miller <mmiller@duosecurity.com>
nsatragno
nsatragno approved these changes Jul 9, 2024
View reviewed changes
Copy link
Member

@nsatragno nsatragno left a comment

Choose a reason for hiding this comment

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

Thank you!

index.bs Outdated Show resolved Hide resolved
@pascoej
Copy link
Contributor

pascoej commented Jul 10, 2024

Thank you!

@emlun emlun merged commit bb9f9bb into main Jul 17, 2024
2 checks passed
@emlun emlun deleted the pr-1951-review-emlun branch July 17, 2024 11:11
github-actions bot added a commit that referenced this pull request Jul 17, 2024
@emlun @github-actions
Merge pull request #2046 from w3c/pr-1951-review-emlun
2feb0c7 
SHA: bb9f9bb
Reason: push, by emlun

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascoej pascoej pascoej approved these changes

@nsatragno nsatragno nsatragno approved these changes

@MasterKale MasterKale MasterKale approved these changes

@timcappalli timcappalli timcappalli approved these changes

@akshayku akshayku Awaiting requested review from akshayku

Assignees
No one assigned
Labels
type:editorial
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@emlun @pascoej @nsatragno @MasterKale @timcappalli