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

Remove tooltip on delete button #6152

Merged
merged 1 commit into from Dec 3, 2015

Conversation

spadgett
Copy link
Member

@spadgett spadgett commented Dec 1, 2015

Allow the first touch to activate the button in mobile Safari.

https://bugzilla.redhat.com/show_bug.cgi?id=1284398

@spadgett
Copy link
Member Author

spadgett commented Dec 1, 2015

@jwforres PTAL

@jwforres
Copy link
Member

jwforres commented Dec 1, 2015

Honestly I'm wondering whether we should even have the tooltip popups on these. Does it really add much? We are prompting you afterward so it doesn't seem that bad.

title="Delete"
aria-hidden="true"></i>
<a class="sr-only" href="" ng-click="openDeleteModal()">Delete {{resourceType | humanizeResourceType}} {{resourceName}}</a>
Copy link
Member

Choose a reason for hiding this comment

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

guessing it looked weird just having a link around the ?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's probably better. I always worry about adding an blue underline or border in some browser.

Copy link
Member Author

Choose a reason for hiding this comment

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

Need to add the action-button class directly to the link or it turns the icon blue.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's fine. And if we remove the hover popup then we can use the
regular title attribute for the sr text.

On Tue, Dec 1, 2015 at 3:33 PM, Sam Padgett notifications@github.com
wrote:

In assets/app/views/directives/delete-button.html
#6152 (comment):

\ No newline at end of file

Need to add the action-button class directly to the link or it turns the
icon blue.


Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/6152/files#r46334568.

@spadgett
Copy link
Member Author

spadgett commented Dec 1, 2015

Yeah I made a similar comment in Bugzilla. I can just remove it.

I'm wondering if we shouldn't avoid popups as much as possible because of issues on mobile like #6153, #5880, and this one.

@spadgett spadgett changed the title Disable delete button tooltip on touch events Remove tooltip on delete button Dec 1, 2015
@spadgett
Copy link
Member Author

spadgett commented Dec 1, 2015

@jwforres Updated to remove the tooltip entirely.

<a href="" ng-click="$event.stopPropagation(); openDeleteModal()" role="button"
><i class="fa fa-trash-o action-button" aria-hidden="true"
></i><span class="sr-only">Delete {{resourceType | humanizeResourceType}} {{resourceName}}</span></a>
Copy link
Member

Choose a reason for hiding this comment

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

I would expect this to just be a title attribute on the tag

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we sure that works across screen readers? I can't find definitively one way or the other. The general advice I see is to avoid title.

@jwforres jwforres added component/web lgtm Indicates that a PR is ready to be merged. labels Dec 2, 2015
@jwforres
Copy link
Member

jwforres commented Dec 2, 2015

[merge]

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@spadgett
Copy link
Member Author

spadgett commented Dec 2, 2015

W1202 10:30:48.766543   17841 pipeline.go:65] Could not find an image stream match for "mysql". Make sure that a Docker image with that tag is available on the node for the build to succeed.
FAIL
coverage: 59.3% of statements
FAIL    github.com/openshift/origin/pkg/generate/app/cmd    15.726s

@jwforres
Copy link
Member

jwforres commented Dec 2, 2015

[merge]

@spadgett
Copy link
Member Author

spadgett commented Dec 2, 2015

re[merge]

Allow the first touch to activate the button in mobile Safari.
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to ebdddbc

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4238/) (Image: devenv-rhel7_2871)

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7625/)

@spadgett
Copy link
Member Author

spadgett commented Dec 3, 2015

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to ebdddbc

openshift-bot pushed a commit that referenced this pull request Dec 3, 2015
@openshift-bot openshift-bot merged commit 693be19 into openshift:master Dec 3, 2015
@spadgett spadgett deleted the delete-tooltip-ios branch December 4, 2015 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/web lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants