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

Add tooltip/ariaLabel for New Button if user has permissions #5155

Merged
merged 1 commit into from
May 27, 2021

Conversation

pascalwengerter
Copy link
Contributor

Description

Add tooltip/ariaLabel for New Button if user has permissions to upload/create resources

@update-docs
Copy link

update-docs bot commented May 26, 2021

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.

@pascalwengerter pascalwengerter requested a review from kulmann May 26, 2021 11:25
if (!this.canUpload) {
return this.$gettext('You have no permission to upload!')
}
if (!this.hasFreeSpace) {
return this.$gettext('You have not enough space left to upload!')
}
return null
return "Add files or folders"
Copy link
Member

Choose a reason for hiding this comment

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

This now adds a tooltip for a non-error case as well. IMO not good. Looks like it's required to separate into two different computed props, one for the tooltip and one for the aria-label.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check the solution I went for and let me know if you like it!

@pascalwengerter pascalwengerter force-pushed the 26052021_add-create-btn-tooltip branch from 6460a4c to 7795fad Compare May 26, 2021 16:28
@pascalwengerter pascalwengerter requested a review from kulmann May 26, 2021 16:28
if (isToolTip === true) {
return null
}
return 'Add files or folders'
Copy link
Member

Choose a reason for hiding this comment

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

This line lacks this.$gettext

}
return null
newButtonTooltip() {
return this.newButtonDescription(true)
Copy link
Member

Choose a reason for hiding this comment

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

newButtonTooltip() could do what we had before:

if (!this.canUpload) {
         return this.$gettext('You have no permission to upload!')
       }
       if (!this.hasFreeSpace) {
         return this.$gettext('You have not enough space left to upload!')
       }
       return null

newButtonTooltip() {
return this.newButtonDescription(true)
},
newButtonAriaLabel() {
Copy link
Member

Choose a reason for hiding this comment

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

... and newButtonAriaLabel() could then do the following:

const tooltip = this.newButtonTooltip
if (tooltip) {
    return tooltip
}
return this.$gettext('Add files or folders')

@@ -306,6 +300,19 @@ export default {
this.createModal(modal)
},

newButtonDescription(isToolTip) {
Copy link
Member

Choose a reason for hiding this comment

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

While this is totally fine technically, it is hard to read in the location where it's called. this.newButtonDescription(true) or this.newButtonDescription(false) doesn't give a hint what effect the boolean has on the output. I'd prefer a solution without the boolean, as pointed out in the two comments above.

@pascalwengerter pascalwengerter force-pushed the 26052021_add-create-btn-tooltip branch from 7795fad to 854f1fc Compare May 27, 2021 07:48
@pascalwengerter pascalwengerter requested a review from kulmann May 27, 2021 07:51
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

LGTM 🤖

@pascalwengerter pascalwengerter merged commit e0353f0 into a11y-swarming May 27, 2021
@pascalwengerter pascalwengerter deleted the 26052021_add-create-btn-tooltip branch May 27, 2021 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants