-
-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Change authorized applications page #17656
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
0e2bc59
to
e58871a
Compare
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.
Not a full review as I have not read the parsing code yet, but I like the idea and the redesign, although I have some comments on the design itself:
- I'm afraid it may take a lot of space per app, but I guess it's manageable
- I think it would be better if there was a clearer visual separation between apps. They don't need to be in the same table.
%strong.announcements-list__item__title | ||
= application.name | ||
- if application.superapp? | ||
%span.account-role.moderator= t('doorkeeper.authorized_applications.index.superapp') |
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.
Not new but we probably shouldn't repurpose all those classes… either rename them to something more neutral or have the styles apply to different classes.
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.
Design-wise, I'd largely prefer if applications were each a different table / at least more visually separated.
Code-wise, I don't like that we are repurposing CSS classes which are now ill-named, but I can't hold that against this PR specifically, this is something that'll have to be addressed separately.
Otherwise this looks good to me!
Last used timestamp would very helpful if user want to get rid of old sessions |
Start tracking when an OAuth access token was last used and from which IP (retain IP data only as long as our general IP retention period allows). Change the design of the authorized applications page in account settings to be more helpful. Change how application permissions are presented.