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

enh(oauth2): allowed toggling of aria label #42132

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Conversation

emoral435
Copy link
Contributor

@emoral435 emoral435 commented Dec 9, 2023

Summary

The button in the table for OAuth 2.0 previously only had the aria label of "Show client secret", even when the button had toggled the rendering of the client secret. Switched aria label to "Hide client secret" when the rendered secret is visible!

TODO

  • add a way to toggle between the states and correctly assign the aria label

Images

Before After
image (image

Checklist

@emoral435
Copy link
Contributor Author

/compile /

@emoral435
Copy link
Contributor Author

/backport to stable28

ShGKme
ShGKme previously requested changes Dec 11, 2023
apps/oauth2/src/components/OAuthItem.vue Outdated Show resolved Hide resolved
@ShGKme ShGKme added this to the Nextcloud 29 milestone Dec 11, 2023
@emoral435 emoral435 dismissed ShGKme’s stale review December 12, 2023 15:18

added computed value for a constant string for community translators to work with 💖

@emoral435
Copy link
Contributor Author

/compile /

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

The icon on this button also has a fixed title.

:title="t('oauth2', 'Show client secret')"

Using title on a span is not correct, it should be on the button and it has same dynamic value with "Hide".

But in this case, I think we can just remove the title attr here.

@emoral435
Copy link
Contributor Author

nice catch! gonna delete that part and then submit the new version : )

@emoral435
Copy link
Contributor Author

/compile /

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Works and seems fine now

apps/oauth2/src/components/OAuthItem.vue Outdated Show resolved Hide resolved
@emoral435 emoral435 force-pushed the fix/client-secret-aria branch 2 times, most recently from 2a82c03 to 33dfc9b Compare December 14, 2023 21:12
@emoral435
Copy link
Contributor Author

@Pytal removed all comments 👍, agree with Grigorii that the name itself is self explanatory!

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

It should have been catched by linter...

apps/oauth2/src/components/OAuthItem.vue Outdated Show resolved Hide resolved
apps/oauth2/src/components/OAuthItem.vue Outdated Show resolved Hide resolved
@emoral435 emoral435 force-pushed the fix/client-secret-aria branch 2 times, most recently from a2f75d1 to 7d9e2c9 Compare December 15, 2023 00:38
Signed-off-by: Eduardo Morales <emoral435@gmail.com>
@emoral435 emoral435 merged commit 29763f0 into master Dec 15, 2023
42 checks passed
@emoral435 emoral435 deleted the fix/client-secret-aria branch December 15, 2023 02:47
@blizzz blizzz mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants