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

update all uikit tooltips to use v-oc-tooltip #5055

Merged
merged 3 commits into from
May 25, 2021
Merged

Conversation

fschade
Copy link
Collaborator

@fschade fschade commented Apr 30, 2021

Description

update all tooltips to use oc-tooltips which will land in the next version of ods (6.4.0)

Related Issue

Motivation and Context

uitooltip is not accessible, a11y audit requires this.

How Has This Been Tested?

  • local installation

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • bump ods to 6.4.0

@fschade fschade self-assigned this Apr 30, 2021
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Very nice! Small nitpick about the package.json/ODS version

packages/web-runtime/package.json Outdated Show resolved Hide resolved
@fschade fschade marked this pull request as ready for review May 4, 2021 09:30
@@ -61,9 +61,9 @@
<div v-if="$_reshareInformation">
<oc-tag
:id="$_resharerToggleId"
v-oc-tooltip="$gettext('Show resharer details')"
Copy link
Member

Choose a reason for hiding this comment

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

Does this tag need a tooltip at all? Usability wise?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't say so. There's tags that are links and there I find tooltips helpful, if it's "just" a small hint I would omit them

pascalwengerter
pascalwengerter previously approved these changes May 7, 2021
@pascalwengerter pascalwengerter dismissed their stale review May 7, 2021 08:14

Took over this PR myself so can't review

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUINotificationBasic failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/15341/

20210507-081448-594.png
20210507-081519-566.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests MarkdownEditor failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/15341/

20210507-081451-629.png
20210507-081522-810.png
20210507-081554-335.png
20210507-081625-645.png
20210507-081718-452.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests TrashbinDelete failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/15341/

20210507-081442-254.png
20210507-081517-797.png
20210507-081552-478.png
20210507-081627-615.png
20210507-081702-082.png
20210507-081736-784.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests TrashbinFilesFolders failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/15341/

20210507-081441-412.png
20210507-081515-508.png
20210507-081549-768.png
20210507-081623-877.png
20210507-081658-763.png
20210507-081732-614.png
20210507-081804-768.png
20210507-081837-284.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests SharingPublicDifferentRoles failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/15341/

20210507-081444-920.png
20210507-081512-819.png
20210507-081541-429.png
20210507-081609-699.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests SharingPublicExpire failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/15341/

20210507-081411-816.png
20210507-081444-613.png
20210507-081516-417.png
20210507-081548-685.png
20210507-081734-300.png
20210507-081809-096.png
20210507-081844-659.png
20210507-081920-986.png
20210507-082015-314.png
20210507-082050-045.png

@@ -7,7 +7,7 @@
class="oc-topbar-menu-burger"
:aria-label="applicationSwitcherLabel"
>
<oc-icon :uk-tooltip="applicationSwitcherLabel" name="apps" class="uk-flex" />
<oc-icon v-oc-tooltip="applicationSwitcherLabel" name="apps" class="uk-flex" />
Copy link
Member

Choose a reason for hiding this comment

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

Tooltip could be moved to the oc-button

@@ -182,6 +183,8 @@ export default {
}
},
canUpload() {
// return this.currentFolder.permissions.indexOf('C') >= 0
Copy link
Member

Choose a reason for hiding this comment

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

Comment (following line as well) can be removed

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 👍

@kulmann kulmann merged commit 72e122b into a11y-swarming May 25, 2021
@delete-merged-branch delete-merged-branch bot deleted the a11y-tooltips branch May 25, 2021 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants