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

fix(seprator): add hasRole input to separator #283

Merged
merged 3 commits into from
Mar 30, 2021

Conversation

elenagarrone
Copy link
Contributor

@elenagarrone elenagarrone commented Mar 26, 2021

Description

Because the lg-separator is mainly used as a decoration we have decided to have it hidden to screen readers by default with the option of adding the role of separator via the input hasRole.

This change will remove the default role from the element but I don't think it's a breaking change to have the role or not.

Link to the discussion: #139

Storybook link: (once netlify has deployed link provide a link to the component)

Checklist:

  • The commit messages follow the convention for this project
  • I have provided an adequate amount of test coverage
  • I have added the functionality to the test app
  • I have provided a story in storybook to document the changes
  • I have provided documentation in the notes section of the story
  • I have added any new public feature modules to public-api.ts
  • I have linked the new component to adobe DSM (if appropriate)

Because the lg-separator is mainly used as a decoration
we have decided to have it hidden to screen readers
by default with the option of adding the role of
separator via the input hasRole.

Link to the discussion: Legal-and-General#139
@elenagarrone elenagarrone requested a review from a team as a code owner March 26, 2021 11:13
@elenagarrone elenagarrone enabled auto-merge (rebase) March 26, 2021 11:14
Copy link
Contributor

@andy-polhill andy-polhill left a comment

Choose a reason for hiding this comment

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

I'm happy this isn't a breaking change. I think currently we have some situations where it is right and some where it is wrong. After this fix that will still be the case, but consumers will at least have the capability to put it right.

@owensgit
Copy link
Contributor

owensgit commented Mar 26, 2021

May be missing something here, but I'm just wondering if we definitely need this to be an input? Would it still work if we manually added the role to the component when using it, without having it as an Angular input - just a regular HTML attribute?

E.g. <lg-seperator role="seperator"></lg-seperator>

If so, part of me is wondering if this is more intuitive? 🤔

What do you think?

@elenagarrone
Copy link
Contributor Author

May be missing something here, but I'm just wondering if we definitely need this to be an input? Would it still work if we manually added the role to the component when using it, without having it as an Angular input - just a regular HTML attribute?

E.g. <lg-seperator role="seperator"></lg-seperator>

If so, part of me is wondering if this is more intuitive? 🤔

What do you think?

I thought about that and decided to go with the input instead. Major reason is that if you add the role yourself you will also have to override the aria-hidden="true" that we set by default and I feel like that is an overhead and very prone to errors. Adding the aria-hidden by default seem the right thing to me as it will stop screen readers reading an unnecessary element.

@elenagarrone elenagarrone merged commit 514253d into Legal-and-General:master Mar 30, 2021
@github-actions
Copy link
Contributor

🎉 This PR is included in version 5.7.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants