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.

Sign up for GitHub

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

Jump to bottom

1455/language tabs a11y #1528

Merged
merged 8 commits into from
Jul 23, 2021
Merged

1455/language tabs a11y #1528

merged 8 commits into from
Jul 23, 2021

Conversation

dominikx96
Copy link
Collaborator

Pull Request Template

Issue

Closes: #1455

Addresses # (issue)

  • This change addresses the issue in full
  • This change addresses only certain aspects of the issue
  • This change is a dependency for another issue
  • This change has a dependency from another issue

Description

Updated list elements to buttons - now these items are focusable and visible for screen readers.

Type of change

  • 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 not work as expected)
  • Prototype/POC (not to merge)
  • This change is a refactor/address technical debt
  • This change requires a documentation update
  • This change requires a SQL Script

How Can This Be Tested/Reviewed?

  1. Open the home page
  2. Try to choose a different language using the keyboard
  • Desktop View
  • Mobile View

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have updated the changelog to include a description of my changes

@dominikx96 dominikx96 self-assigned this Jul 19, 2021
@netlify
Copy link

netlify bot commented Jul 19, 2021 •
edited
Loading

✔️ Deploy Preview for jovial-davinci-1d67a0 ready!

🔨 Explore the source changes: 4c44191

🔍 Inspect the deploy log: https://app.netlify.com/sites/jovial-davinci-1d67a0/deploys/60fae92abafe600008f2f59e

😎 Browse the preview: https://deploy-preview-1528--jovial-davinci-1d67a0.netlify.app

@netlify
Copy link

netlify bot commented Jul 19, 2021 •
edited
Loading

✔️ Deploy Preview for dev-partners-bloom ready!

🔨 Explore the source changes: 4c44191

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/60fae92ac6dc9a000712743e

😎 Browse the preview: https://deploy-preview-1528--dev-partners-bloom.netlify.app

@netlify
Copy link

netlify bot commented Jul 19, 2021 •
edited
Loading

✔️ Deploy Preview for dev-storybook-bloom ready!

🔨 Explore the source changes: 4c44191

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/60fae92aef1a420007c72d2e

😎 Browse the preview: https://deploy-preview-1528--dev-storybook-bloom.netlify.app

@netlify
Copy link

netlify bot commented Jul 19, 2021 •
edited
Loading

✔️ Deploy Preview for dev-bloom ready!

🔨 Explore the source changes: 4c44191

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/60fae92ae2ff450007cb6a04

😎 Browse the preview: https://deploy-preview-1528--dev-bloom.netlify.app

emilyjablonski
emilyjablonski approved these changes Jul 20, 2021
View reviewed changes
Copy link
Collaborator

@emilyjablonski emilyjablonski left a comment

Choose a reason for hiding this comment

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

LGTM

@seanmalbert seanmalbert requested a review from akegan July 20, 2021 21:31
@slowbot
Copy link
Collaborator

slowbot commented Jul 21, 2021

@dominikx96 The preview in storybook looks broken /story/navigation-languagenav--default

@seanmalbert
Copy link
Collaborator

@akegan ,
When you get a chance, can you please review.

@seanmalbert seanmalbert removed their request for review July 21, 2021 22:26
akegan
akegan suggested changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

@akegan akegan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM on the homepage and code, but I'm seeing this error on the storybook preview site

image

I actually see the same issue on storybook.bloom.exygy.dev, so if you think it's out of scope to fix in this PR I can approve as-is, however, it'd also be nice if the a11y tests could run on this component which requires a working storybook story.

@dominikx96
Copy link
Collaborator Author

@akegan I've fixed the storybook component, can you make sure that everything works as expected? Thanks!

@dominikx96 dominikx96 requested a review from akegan July 23, 2021 16:08
akegan
akegan approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@akegan akegan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing the storybook component!
image

@seanmalbert seanmalbert merged commit 7be8e51 into dev Jul 23, 2021
@dominikx96 dominikx96 mentioned this pull request Jul 27, 2021
Closed
seanmalbert referenced this pull request in CityOfDetroit/bloom Jun 23, 2022
@dominikx96 @lint-action
1455/language tabs a11y (#1528)
b684517 
* Add button element to follow a11y rules

* Update styles

* Update changelog

* Fix code style issues with Prettier

* Fix test

* Fix storybook component props

Co-authored-by: Dominik Barcikowski <dominik@airnauts.com>
Co-authored-by: Lint Action <lint-action@samuelmeuli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akegan akegan akegan approved these changes

@emilyjablonski emilyjablonski emilyjablonski approved these changes

Assignees

@dominikx96 dominikx96

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dominikx96 @slowbot @seanmalbert @akegan @emilyjablonski @lint-action