Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: add user/settings page for managing external auth#10945

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Emyrk merged 9 commits intomainfromexternal_auth_fe_updates
Dec 6, 2023

Conversation

Emyrk
Copy link
Member

@EmyrkEmyrk commentedNov 29, 2023
edited
Loading

FE team, if you have comments/suggestions that are quick, feel free to submit to this PR. This FE took me quite some time to get right. There is still the issue of a redundant fetch, and it could be styled more.

Screencast.from.2023-12-05.10-22-07.webm

matifali reacted with heart emoji
@EmyrkGraphite App
Copy link
MemberAuthor

Emyrk commentedNov 29, 2023
edited
Loading

@EmyrkEmyrkforce-pushed theexternal_auth_be_updates branch 3 times, most recently from21dcf27 to6111d53CompareNovember 30, 2023 18:26
@EmyrkEmyrkforce-pushed theexternal_auth_be_updates branch 8 times, most recently froma9b2119 toc02c754CompareDecember 1, 2023 15:46
@EmyrkEmyrkforce-pushed theexternal_auth_fe_updates branch from6a98155 tob9217dcCompareDecember 1, 2023 15:58
@EmyrkEmyrk marked this pull request as ready for reviewDecember 5, 2023 16:23
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment

Choose a reason for hiding this comment

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

About code: I left a few considerations about mutation usage

About design:

  • The table should be wider
  • I feel the button is taller than it should be

@aslilac
Copy link
Member

aslilac commentedDec 5, 2023
edited
Loading

I think the whole table kind of feels "zoomed in" compared to the chrome around it. I guess mainly the icon and button are both way too big. Might be worth referencing the licenses page, it has kind of a similar vibe to this one. (although it actually kind of feels too big too...hmm)

@Emyrk
Copy link
MemberAuthor

I think the whole table kind of feels "zoomed in" compared to the chrome around it. I guess mainly the icon and button are both way too big. Might be worth referencing the licenses page, it has kind of a similar vibe to this one. (although it actually kind of feels too big too...hmm)

Yea, the look isoff for sure. I spent some time trying to fix this, but at some point I figured someone else can do a much better job in much less time than myself 😄

So I just focused on the functionality.

Base automatically changed fromexternal_auth_be_updates tomainDecember 5, 2023 20:03
@matifali
Copy link
Member

matifali commentedDec 5, 2023
edited
Loading

This is a huge improvement. Thanks@Emyrk 😊.
I think we should not have the confirmation dialogue as a user can authenticator again very easily. It's not something very dangerous like Deleting a workplace or template.

@Emyrk
Copy link
MemberAuthor

This is a huge improvement. Thanks@Emyrk 😊. I think we should not have the confirmation dialogue as a user can authenticator again very easily. It's not something very dangerous like Deleting a workplace or template.

One benefit to the confirm page is it educates the user that the "unlink" is only on our side. They need to revoke from the application to truly invalidate the token.

matifali reacted with thumbs up emoji

@matifali
Copy link
Member

I agree. Are there ways we can invalidate the token on our end? By sending a request to the provider?
Also this warning could also be moved as info text somewhere on the revoke button.
I feel its uncecarsy to type the name of the provider Integration.
Moreover, a simple reuthenticate button without unlinking would help if for some reason a user needs a new token.

@EmyrkEmyrkforce-pushed theexternal_auth_fe_updates branch fromf6d8c53 tocc7cd52CompareDecember 5, 2023 21:01
@Emyrk
Copy link
MemberAuthor

I agree. Are there ways we can invalidate the token on our end? By sending a request to the provider?

There is an RFC for an oauth2 extension:https://datatracker.ietf.org/doc/html/rfc7009

It is not part of the main spec though so, "your mileage may vary" based on the provider. Since we have providertypes, we can implement them based on the provider type. I think for a first draft this is ok.

Also this warning could also be moved as info text somewhere on the revoke button. I feel its uncecarsy to type the name of the provider Integration. Moreover, a simple reuthenticate button without unlinking would help if for some reason a user needs a new token.

Fair. I think this ui is very first draft.

@Emyrk
Copy link
MemberAuthor

Emyrk commentedDec 5, 2023
edited
Loading

About code: I left a few considerations about mutation usage

About design:

  • The table should be wider
  • I feel the button is taller than it should be

Is this something we could fix in a follow up?@BrunoQuaresma

@aslilac
Copy link
Member

is this behind an experiment? if so fixing later is fine, if not I think I'd rather land it in better shape since the changes would be very minor

@Emyrk
Copy link
MemberAuthor

is this behind an experiment? if so fixing later is fine, if not I think I'd rather land it in better shape since the changes would be very minor

It's not behind an experiment.

@aslilac do you or@BrunoQuaresma have some time to clean it up in this PR?

@BrunoQuaresma
Copy link
Collaborator

@steve-Coder-Ent yeap, I can do it tomorrow morning. In case I miss that, can you ping me on Slack to remind me pls 🙏 ?

matifali and aslilac reacted with heart emoji

@Emyrk
Copy link
MemberAuthor

@steve-Coder-Ent yeap, I can do it tomorrow morning. In case I miss that, can you ping me on Slack to remind me pls 🙏 ?

Perfect thanks! 🌮

@BrunoQuaresma
Copy link
Collaborator

@steve-Coder-Ent I think it is just easier to merge it and I work on top of it so I can use the BE changes directly from dev.coder.com. Sounds good for you?

@EmyrkEmyrk merged commitb376b2c intomainDec 6, 2023
@EmyrkEmyrk deleted the external_auth_fe_updates branchDecember 6, 2023 14:41
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsDec 6, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@BrunoQuaresmaBrunoQuaresmaBrunoQuaresma approved these changes

@aslilacaslilacAwaiting requested review from aslilac

Assignees

@EmyrkEmyrk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@Emyrk@aslilac@matifali@BrunoQuaresma

[8]ページ先頭

©2009-2025 Movatter.jp