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 HOWTO-chrome.md with button flow dev trial instruction #551

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

yi-gu
Copy link
Collaborator

@yi-gu yi-gu commented Mar 19, 2024

No description provided.

yi-gu added 2 commits March 4, 2024 12:44
Update HOWTO-chrome.md with button flow and use other account dev trial instruction
@yi-gu yi-gu requested a review from npm1 March 19, 2024 13:52
Copy link
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

small tweaks for clarity

}
```

Note: IdP can add "widget" in "modes" to support this feature in the non-button flow. However,
Copy link
Collaborator

Choose a reason for hiding this comment

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

But does it actually work? If not, then mention that it does not work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It works. One can trigger the "use other account" row in the widget flow if they specify it in the config file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So it works with the chrome://flags but it won't work with the OT token? Does this mean we need a code change for this? We should probably not add this to the instructions if it is just missing in the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO we don't need a code change for this. This instruction is for dev trialing the "Use Other Account" API so that we should keep the function. Agreed that we can remove this note.

@yi-gu
Copy link
Collaborator Author

yi-gu commented Mar 20, 2024

PTAL. Thanks!


### Button Flow
The button flow differs from the widget flow in several ways. The most significant
difference is that the button flow is on the critical path of a user's sign-in
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems speculative.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Updated.

}
```

Note: IdP can add "widget" in "modes" to support this feature in the non-button flow. However,
Copy link
Collaborator

Choose a reason for hiding this comment

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

So it works with the chrome://flags but it won't work with the OT token? Does this mean we need a code change for this? We should probably not add this to the instructions if it is just missing in the code.

Copy link
Collaborator Author

@yi-gu yi-gu left a comment

Choose a reason for hiding this comment

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

Thanks! PTAL.

}
```

Note: IdP can add "widget" in "modes" to support this feature in the non-button flow. However,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO we don't need a code change for this. This instruction is for dev trialing the "Use Other Account" API so that we should keep the function. Agreed that we can remove this note.


### Button Flow
The button flow differs from the widget flow in several ways. The most significant
difference is that the button flow is on the critical path of a user's sign-in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Updated.

@npm1 npm1 merged commit 40e9897 into w3c-fedid:main Mar 25, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Mar 25, 2024
SHA: 40e9897
Reason: push, by npm1

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to mattdanielbrown/WebID that referenced this pull request Mar 26, 2024
…did#551)

SHA: 40e9897
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to cbiesinger/WebID that referenced this pull request Apr 16, 2024
…did#551)

SHA: 40e9897
Reason: push, by cbiesinger

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
npm1 pushed a commit that referenced this pull request Jul 31, 2024
* Update HOWTO-chrome.md with button flow dev trial instruction

* Apply suggestions from code review

Co-authored-by: Ted Thibodeau Jr <[email protected]>

* Update HOWTO-chrome.md

* Address comments

---------

Co-authored-by: Ted Thibodeau Jr <[email protected]>
npm1 pushed a commit that referenced this pull request Sep 18, 2024
* Update HOWTO-chrome.md with button flow dev trial instruction

* Apply suggestions from code review

Co-authored-by: Ted Thibodeau Jr <[email protected]>

* Update HOWTO-chrome.md

* Address comments

---------

Co-authored-by: Ted Thibodeau Jr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants