-
Notifications
You must be signed in to change notification settings - Fork 2
Display collection without logging in #49
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
Conversation
andresbono
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When building the container image, tsc raised a few errors: error TS7006: Parameter 'XXXXX' implicitly has an 'any' type. I updated the compilation settings to bypass, but maybe we should look into those errors
| } | ||
| } | ||
|
|
||
| const AuthnWrapper = ({ children } : { children: ReactElement }) => disabled ? <Tooltip title='Log in to install this application'>{ children }</Tooltip> : <>{ children }</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are not prompted directly to the login form, does it make sense to complete a little bit how to do so?
| const AuthnWrapper = ({ children } : { children: ReactElement }) => disabled ? <Tooltip title='Log in to install this application'>{ children }</Tooltip> : <>{ children }</> | |
| const AuthnWrapper = ({ children } : { children: ReactElement }) => disabled ? <Tooltip title='Go to the extension settings and log in to install this application'>{ children }</Tooltip> : <>{ children }</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to invite the user to login via notifications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, I've pushed 7af2c24 so the initial notification encourages the users to log in.
Co-authored-by: Andrés Bono <[email protected]>
@andresbono I'd say that's related to the autogenerated backend clients. Try running |
Thank you. A rebase from |
…extension into display-collection-without-loggin-in
| type Auth = string | null | undefined | ||
|
|
||
| type ReducerAction = { | ||
| type: 'set' | 'update' | 'delete' | 'dismiss_errors', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated, but this was unused
andresbono
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is a proposal to satisfy the discussion started here: #48.
The navigation is enabled without authentication required. However, when the user reaches the point where authentication is necessary, the actionable cards/buttons are disabled with a tooltip:
Once the user goes to
Settingsand configures valid credentials, these actions are enabled.