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

Add PromptOAuth component #44

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Add PromptOAuth component #44

merged 1 commit into from
Sep 16, 2024

Conversation

buckett
Copy link
Member

@buckett buckett commented Sep 12, 2024

This allows for the UI of a tool to continue to be shown, but for a prompt to grant access to be overlaid on-top.

This was added as a new component as it's behaviour is quite different to the existing OAuth component in that it doesn't have any child components.

This allows for the UI of a tool to continue to be shown, but for a prompt to grant access to be overlaid on-top.

This was added as a new component as it's behaviour is quite different to the existing OAuth component in that it doesn't have any child components.
</Modal.Header>
<Modal.Body>
<View as='div'>
Please grant permission to load this content. This is a one-time (two-step) authorisation.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence I had to read twice , since for me brackets imply non-necessary further detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, I don't like the message at all.

// what content is waiting on the OAuth grant. For example in the Module Titles
// there may be other content in the page that doesn't require the granting of
// permission
background: 'rgba( 200, 200, 200, 0.5 )'
Copy link
Contributor

Choose a reason for hiding this comment

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

css or we do stuff inline too these days if it's one-off/small?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, what are you suggesting here? We need to override the theme variables as documented: https://instructure.design/#using-theme-overrides

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok

// This is to grey out the background behind the modal more so that it's clear
// what content is waiting on the OAuth grant. For example in the Module Titles
// there may be other content in the page that doesn't require the granting of
// permission
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice explanation

Copy link
Contributor

@nicholaswilson100 nicholaswilson100 left a comment

Choose a reason for hiding this comment

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

lgtm

@buckett buckett merged commit 4ed928e into master Sep 16, 2024
1 check passed
@buckett buckett deleted the prompt-oauth branch September 16, 2024 08:45
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.

2 participants