Skip to content

Conversation

@yeion7
Copy link
Contributor

@yeion7 yeion7 commented Oct 4, 2019

What kind of change does this PR introduce?

Refactor

What is the current behavior?

The notifications has a few a11 issues

What is the new behavior?

What steps did you take to test this?

Checklist

  • Documentation
  • [] Testing
  • Ready to be merged
  • Added myself to contributors table

@vercel vercel bot temporarily deployed to staging October 4, 2019 13:08 Inactive
@vercel
Copy link

vercel bot commented Oct 4, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://codesandbox-client-git-fork-yeion7-notifications-a11y.codesandbox1.now.sh

@vercel vercel bot temporarily deployed to staging October 7, 2019 17:00 Inactive
@vercel vercel bot temporarily deployed to staging October 7, 2019 20:46 Inactive
@yeion7
Copy link
Contributor Author

yeion7 commented Oct 7, 2019

@SaraVieira 👋 this PR is ready for review 😁

btw, thanks for review my last PRs

Copy link
Contributor

@SaraVieira SaraVieira left a comment

Choose a reason for hiding this comment

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

One day people CodeSandbox without a mouse and it will be thanks to you

@SaraVieira SaraVieira merged commit a90137e into codesandbox:master Oct 7, 2019
CompuIves added a commit that referenced this pull request Oct 8, 2019
@CompuIves
Copy link
Member

I reverted this PR because I noticed some issues in the dashboard. It seems like the z-index wasn't set correctly:
image

I also noticed that the performance of the modal itself went down a bit when moving the mouse over the items, this could either be because of the focus outline that pops in and out or something else.

I also noticed this:

image (4)

Is there a way to make this more beautiful? Might need @DannyRuchtie input here, but I find the border to be quite ugly, and while I do see the usefulness from an a11y perspective here, I also think we can do better than just adding a blue border.

@yeion7
Copy link
Contributor Author

yeion7 commented Oct 8, 2019

oh no 😱

@CompuIves these problems happened in your local environment? seems like you have 1.0.0-beta.4, but this PR upgrade to 1.0.0-beta.8, this version fixed the z-index issue and also the focus is set in the first focusable element and not the container, could you upgrade your dependencies and let me know if issues persist?

The focus style, I think is important to keep it, more details here,

Maybe we could implement this trick for outline when use mouse, while we have a better outline

:focus:not(:focus-visible) {
  outline: none;
}

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.

3 participants