-
Notifications
You must be signed in to change notification settings - Fork 230
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
Click on a read notification just leaves focus on the notification #2293
Comments
@openshift/team-ux-review |
@jeff-phillips-18 I don't think we have this issue in PatternFly, but curious if you have seen it? Typically if a row is already marked as read, we would not see the blue outline. @jwforres I agree it is unnecessary. |
Yeah @spadgett thought this was probably a bug in our special handling of click events in our impl of the drawer. Once its been marked as read I think we should remove the click handler and click target (no longer show the hand when hovering). |
It's specific to OpenShift notifications: |
We can remove that handler pretty easily:
There is no longer a click, but that doesn't affect the styling. Working on a solution. |
Looking @ this with @sg00dwin. So the patternfly demo appears to be tinkering with the focus. If I manually trigger the "focus" attrib in the web inspector, I can achieve the same effect: So there is something happening in the demo CSS that is not inherited in the actual component that is causing the focus to be hidden or clipped. The property is |
The cause is our
We did this to add the ability to tab through the notifications. The patternfly / angular-patternfly implementations do not do this. Visually it is a tad annoying, but what is the best solution regarding accessibility? Default patternfly only has focus on the kebabs: |
I don't think what we have no is accessible anyway since there is no Better to have an |
Thats true, the I can do: <button
class="sr-only"
ng-click="notification.unread && $ctrl.customScope.markRead(notification)">
Mark Read
</button> It eliminates the focus. Its a bit of a strange experience in that you can tab to this button, but since it is hidden you don't get any visual feedback. |
Maybe add https://getbootstrap.com/docs/3.3/css/#helper-classes-screen-readers |
Yeah we probably want to put it at the bottom of the notification content with good margins since it will be visible when focused. |
…on-click-handler Automatic merge from submit-queue. Fix issue 2293: click on read notification focus bug Fix issue #2293 You can still click the notification to clear it, but now there is a `sr-only` button for tabbing. The gif below shows the tabbing experience. There is clearly a hidden element in the flow. ![2017-10-18 14 08 43](https://user-images.githubusercontent.com/280512/31734870-0e00b0b8-b40e-11e7-9989-29a3a06f58cc.gif) @jwforres @spadgett
We still have a bug here that the hover still makes it look clickable even after the notification is read, the cursor needs to change to a pointer. |
The current behavior is what UXD is asking for, so closing. We can address any remaining concerns upstream in patternfly. |
Right now if you click on an already read notification it looks like this (see blue focus outlines):
I question whether we should even still have a click handler on a read notification, clicking it does absolutely nothing.
The text was updated successfully, but these errors were encountered: