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

feat: improve Authentication Providers page #5424

Merged

Conversation

axel7083
Copy link
Contributor

@axel7083 axel7083 commented Jan 4, 2024

What does this PR do?

This PR introduce small change to the Authentification Providers page

Screenshot / video of UI

Before After
image image
image image
image image

What issues does this PR fix or reference?

Related to #2274

  • Display all accounts instead of only one
  • Allow to sign out of individual account instead of only the first one
  • Add tooltip for sign out

How to test this PR?

You can use the #5400 to test an authentification provider

CC @mairin :)

@axel7083 axel7083 marked this pull request as ready for review January 4, 2024 16:58
@axel7083 axel7083 requested review from benoitf and a team as code owners January 4, 2024 16:58
@axel7083 axel7083 requested review from jeffmaury and cdrage and removed request for a team January 4, 2024 16:58
@dgolovin dgolovin requested a review from mairin January 4, 2024 17:31
Copy link
Contributor

@dgolovin dgolovin left a comment

Choose a reason for hiding this comment

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

LGTM

@axel7083 axel7083 self-assigned this Jan 5, 2024
@axel7083 axel7083 linked an issue Jan 5, 2024 that may be closed by this pull request
@deboer-tim
Copy link
Collaborator

👍🏼 to change. 2 nits though:

  • Line 26 - since we're changing it, can you replace with the class instead of style?
  • The drop down menu looks out of place now (doesn't look like a button next to the accounts) and 'moves' the accounts left when it exists. For the first it might help to give it a bg, but that might not be necessary if you (e.g.) put the accounts in a middle column so that they're always aligned.

We should really switch this to use IconImage too, but that's a different PR.

@axel7083
Copy link
Contributor Author

  • The drop down menu looks out of place now (doesn't look like a button next to the accounts) and 'moves' the accounts left when it exists. For the first it might help to give it a bg, but that might not be necessary if you (e.g.) put the accounts in a middle column so that they're always aligned.

@deboer-tim I updated the mockups and the code

Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083 axel7083 force-pushed the feature/improve-auth-provider-page branch from 997fa16 to d61300d Compare January 15, 2024 12:54
Copy link
Collaborator

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. One minor nit but LGTM.

<span class="my-auto font-bold col-span-1 text-right">
{account.label}
</span>
<Tooltip tip="Sign out of {account.label}" left>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can just give the button a title (which is used as a tooltip) instead of using a Tooltip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

The button tooltip is a bit unstyled (ugly), do we want to use that ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't really have a standard, and have used button/other tooltip for most simple cases and Tooltip for cases where it isn't supported or needs more styling. I'm fine leaving it, let's merge.

@deboer-tim deboer-tim merged commit 49ecb3a into containers:main Jan 17, 2024
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.7.0 milestone Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Improve logout action, sync
4 participants