-
Notifications
You must be signed in to change notification settings - Fork 159
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
Hide private links based on capabilities, refactor spans in sidebar t… #5034
Hide private links based on capabilities, refactor spans in sidebar t… #5034
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
@@ -140,8 +141,8 @@ | |||
<div class="uk-flex uk-flex-nowrap uk-flex-middle"> | |||
<oc-button | |||
v-if="$_editButtonVisible" | |||
:aria-label="$gettext('Edit share')" | |||
:uk-tooltip="$gettext('Edit share')" | |||
:aria-label="$gettext(`Edit share with ${ collaborator.collaborator.displayName }`)" |
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.
Unfortunately doesn't work like this. For inserting a variable into a translation, you need to use gettextInterpolate
. And that's a 2-step-process. Translating the string (having a placeholder in the string) and then injecting the value into the translated string, which replaces the placeholder. See docs: https://github.com/Polyconseil/vue-gettext#interpolation-support-1 - I'd recommend to do this in a computed property.
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.
Yeah I recall seeing this somewhere in the codebase, will push an update later today! Thanks ;)
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.
👍
@@ -39,7 +37,7 @@ export default { | |||
}), | |||
|
|||
computed: { | |||
...mapGetters('Files', ['highlightedFile']), | |||
...mapGetters('Files', ['highlightedFile'], ['capabilities']), |
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.
mapGetters
expects two params: the namespace (can be omitted for root level) and one array or object. Please merge the two getter names into one array. ;-)
1b3d4b9
to
702ea81
Compare
<oc-spinner | ||
v-if="loading" | ||
key="avatar-loading" | ||
:aria-label="$gettext('Loading')" | ||
:aria-label="$gettext('Loading user avatar')" |
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.
you can remove the aria-label entirely since the wrapper is aria-hidden
702ea81
to
07ce4b9
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.
LGTM 🥇
07ce4b9
to
03bec6f
Compare
- Gave `role="presentation" to the collaborator avatar | ||
- Refactored `<span>` and `<div>` tags into `<p>` tags and unified translations a bit | ||
- Enhanced hints in the collaborator quick action buttons with collaborator name | ||
- Hide private links if the capability is not enabled |
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.
IMO this is worth its own bugfix changelog
03bec6f
to
319c7dd
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.
LGTM (again) 👍
Description
This is a WIP PR, I'll push some more changes and add a changelog item later. Feel free to contribute suitable changes!