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

Icon picker: Accessibility improvements #6990

Merged
merged 3 commits into from
Jan 13, 2020
Merged

Icon picker: Accessibility improvements #6990

merged 3 commits into from
Jan 13, 2020

Conversation

BatJan
Copy link
Contributor

@BatJan BatJan commented Oct 30, 2019

Prerequisites

  • I have added steps to test this contribution in the description below

Description

In this PR I have done the following

  • Changed <a> to <button>
  • Fixed styling when <button is used
  • Hidden icon using aria-hidden="true"
  • Visually hidden text that will announce "Select (icon-name)"

Visually everything looks the same as before 😃

@poornimanayar
Copy link
Contributor

Hi @BatJan ,

Thanks yet again!

Poornima

@kjac kjac changed the base branch from v8/dev to v8/contrib December 21, 2019 18:02
Copy link
Contributor

@kjac kjac left a comment

Choose a reason for hiding this comment

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

@BatJan this works just fine 👍

If I'm not mistaken there's really no reason to leave behind the .umb-iconpicker-item a rules in the LESS file, is there? I can't find anywhere else in the codebase using .umb-iconpicker-item, and with these changes there's no longer any a tag in there.

But maybe you had a good reason to keep those rules in place?

@kjac
Copy link
Contributor

kjac commented Jan 13, 2020

I went ahead and deleted those obsolete styles to get this merged in.

@kjac kjac merged commit d48b33d into umbraco:v8/contrib Jan 13, 2020
@kjac
Copy link
Contributor

kjac commented Jan 13, 2020

...aaaand merged 👍 thanks again, @BatJan

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

Successfully merging this pull request may close these issues.

3 participants