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

Implement accessible tabs #1

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

sjsyrek
Copy link

@sjsyrek sjsyrek commented Jun 28, 2019

This is a very basic implementation with no bells and whistles whatsoever. In response to https://dev.to/stereobooster/can-you-create-react-accessible-component-45j1.

package-lock.json Outdated Show resolved Hide resolved
@stereobooster
Copy link
Owner

Probably travis fails due to misconfiguration https://docs.cypress.io/guides/guides/continuous-integration.html#Travis. You can try to fix it or I will do it later myself

.travis.yml Outdated Show resolved Hide resolved
@stereobooster
Copy link
Owner

Code looks reasonable. It can happen that I wrote wrong tests. Need to investigate

@sjsyrek
Copy link
Author

sjsyrek commented Jun 28, 2019

@stereobooster I have been looking at the tests, and it seems like they should pass, but I am not familiar with the Cypress API, so I don't know why it can't find the elements it's looking for.

src/App.js Outdated Show resolved Hide resolved
@stereobooster
Copy link
Owner

stereobooster commented Jun 28, 2019

in the first console

yarn start

in the second console

yarn cypress open

and see tests running in the browser

@stereobooster
Copy link
Owner

stereobooster commented Jun 30, 2019

Here is one of the ways how to fix it 3aca842

UPD I fixed tests in master. Remove latest commit from this PR (so you would not have conflicts) and rebase

@sjsyrek
Copy link
Author

sjsyrek commented Aug 10, 2019

@stereobooster I added the delete function and changed a few of the the timeouts in the tests

break;
case "Delete":
e.preventDefault();
setActiveTab(activeTab === 0 ? content.length - 1 : activeTab - 1);
Copy link
Owner

Choose a reason for hiding this comment

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

Why make tab active before deleting it? I wonder where focus will go after we delete element from the DOM 🤔

key={i}
index={i}
selected={activeTab === i}
id={tab.title.split(" ").join("-")}
Copy link
Owner

Choose a reason for hiding this comment

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

if tabs will have the same names (why not?) it will result in duplicate IDs

type="button"
aria-selected={selected}
aria-controls={`${id}-tab`}
id={`${id}-tab`}
Copy link
Owner

Choose a reason for hiding this comment

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

      aria-controls={`${id}-tab`}
      id={`${id}-tab`}

button controls itself 🤔

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.

None yet

2 participants