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

feat(VIcon): Add 'tag' prop to VIcon that sets a custom tag to be used #6899

Merged
merged 5 commits into from
Apr 3, 2019

Conversation

FRFlor
Copy link
Contributor

@FRFlor FRFlor commented Apr 2, 2019

Description

Add a new optional prop to VIcon, the 'tag' prop, that specifies a custom tag to use in the
component, similar to the tag prop from VContainer.

For the sake of backwards compatibility with existing Vuetify projects, this PR keeps the standard behavior of using the <i> tag if none is provided. However, it also gives the option for the developer to pass a new tag prop. If this prop is provided, the V-icon component will use the element specified as the wrapper.

Motivation and Context

The usage of <i> is not recommended under the guidelines of accessibility (WCAG A/AA), since the<i> tag is not handled well by some screenreaders.

Even though these <i> tags are being used for icons on the VIcon component, this still leads to warnings on Accessibility tests.

Using the Accessibility Checker website, we can see multiple instances of errors from the icons used on the page.

https://achecker.ca/ .

On either https://vuetifyjs.com/en/ or https://www.felipeflor.com will show that every instance of v-icon being used triggers a warning.

image

How Has This Been Tested?

The changes have been implemented with a TDD approach. Two new unit tests have been added with this new commit, one that assures the component will remain using the <i> tag if none is provided, and another test that assures the component will use any given tag provided by the developer.

Types of changes

  • 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)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes and documentation updates, dev for new features and breaking changes).
  • My code follows the code style of this project.
  • I've added relevant changes to the documentation (applies to new features and breaking changes in core library)

@codecov
Copy link

codecov bot commented Apr 2, 2019

Codecov Report

Merging #6899 into next will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6899      +/-   ##
==========================================
+ Coverage   81.85%   81.86%   +<.01%     
==========================================
  Files         329      329              
  Lines        8438     8437       -1     
  Branches     2138     2138              
==========================================
  Hits         6907     6907              
+ Misses       1440     1439       -1     
  Partials       91       91
Impacted Files Coverage Δ
packages/vuetify/src/components/VIcon/VIcon.ts 100% <100%> (ø) ⬆️
...tify/src/components/VAutocomplete/VAutocomplete.ts 99.22% <0%> (-0.01%) ⬇️
packages/vuetify/src/mixins/overlayable/index.ts 34.44% <0%> (+1.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c006f1...0b2baad. Read the comment docs.

@vercel
Copy link

vercel bot commented Apr 2, 2019

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

Copy link
Contributor

@dsseng dsseng left a comment

Choose a reason for hiding this comment

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

@FRFlor please rebase to next

Add a new optional prop to VIcon, the 'tag' prop, that specifies a custom tag to use in the
component, similar to the tag prop from VContainer. If none is provided, the component will fallback
to use <i> as the tag, for the benefit of backwards compatibility.
@FRFlor
Copy link
Contributor Author

FRFlor commented Apr 2, 2019

@sh7dm Rebased with next

@dsseng
Copy link
Contributor

dsseng commented Apr 2, 2019

@FRFlor thanks. Have you tested it with different icon sets such as Material Icons, Material Design Icons, Font Awesome 4/5 and SVG?

@dsseng dsseng self-requested a review April 2, 2019 15:04
@TravisBuddy
Copy link

TravisBuddy commented Apr 2, 2019

Hey @FRFlor,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 5bf32680-55c9-11e9-bc42-792bcead230c

Fix linting error by removing the trailing comma
@FRFlor
Copy link
Contributor Author

FRFlor commented Apr 2, 2019

@FRFlor thanks. Have you tested it with different icon sets such as Material Icons, Material Design Icons, Font Awesome 4/5 and SVG?

I'll do a more thorough test and will let you know.

@FRFlor
Copy link
Contributor Author

FRFlor commented Apr 3, 2019

@sh7dm
I did more tests on a personal project and the component seems to be working fine regardless of the source of the icons.

Everything seems okay.

Thanks for all the help, really appreciated it!

@dsseng dsseng merged commit 6504a7a into vuetifyjs:next Apr 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants