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 linting error in ft-input-tags.vue #2971

Merged

Conversation

petaded
Copy link
Contributor

@petaded petaded commented Dec 17, 2022

fix linting error in ft-input-tags.vue

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Linting is currently failing after #2849

Description

I don't quite understand why the lint error was not caught on the auto running checks on PR's.
Either way here is a fix for the linting error currently on the development branch.

Here is the reported error on dev branch:

/home/runner/work/FreeTube/FreeTube/src/renderer/components/ft-input-tags/ft-input-tags.vue
Error:   23:11  error  Form label must have an associated control  vuejs-accessibility/label-has-for

✖ 1 problem (1 error, 0 warnings)

Screenshots

Testing

(dev/petaded/fix_linting_error)> npm run lint

> [email protected] lint
> eslint --ext .js,.vue ./

Also tested the feature still works as expected. I added two channels to block. Then removed them.

Desktop

  • OS: manjaro
  • OS Version: Kernel: 5.15.78-1-MANJARO
  • FreeTube version: 0.18.0

Additional context

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 17, 2022
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 17, 2022 23:44
PikachuEXE
PikachuEXE previously approved these changes Dec 18, 2022
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

image

@absidue
Copy link
Member

absidue commented Dec 18, 2022

the for attribute needs to be an id of an html element, this PR fixes the problem enough so that the linting error doesn't show up, but doesn't actually fix the accessibility issue as the label doesn't point to anything.

the label html element is always meant to be used in tandem with a html form element, e.g. showing a label next to a text field, in this case it would probably be better to switch to a span element, as you just want to display some text, you aren't trying to label a form field.

auto-merge was automatically disabled December 18, 2022 14:22

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 18, 2022 14:22
@petaded
Copy link
Contributor Author

petaded commented Dec 18, 2022

I had wondered at that but didn't know the right thing to do.

Thanks for the help. Have updated it to use a span and updated the css accordingly.

Linting still passes.

 (dev/petaded/fix_linting_error) [SIGINT]> npm run lint

> [email protected] lint
> eslint --ext .js,.vue ./

And tag box still works

image

@@ -20,7 +20,7 @@
v-for="tag in tagList"
:key="tag.id"
>
<label>{{ tag }}</label>
<span>{{ tag }}</span>
<font-awesome-icon
:icon="['fas', 'fa-times']"
Copy link
Member

Choose a reason for hiding this comment

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

We should probably also add a ':title' and an ":aria-label' to this element that will be "Remove {tag}" so people with screen readers can tell that this is a remove button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChunkyProgrammer et al,

Do you mind if I do the screenreader accessibility update in a separate PR. This one atm fixes the issue its supposed to and It looks like it will take me a while to sort a screenreader fix.

Currently struggling to get a screenreader to work with freetube at all atm. I've got it reading out normal OS menu's etc but as soon as I tab to freetube it stops saying anything - same thing happens with brave, but some programs work like my password safe.

for reference i'm using Orca with espeak ( the built in linux screenreader with gnome )

Copy link
Member

Choose a reason for hiding this comment

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

@petaded I understand you wanting to test accessibility changes, however as adding the title and aria-label attributes is standard web accessibility stuff, I think it's fine to do it without testing it with a screen reader.

Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

As the linting error is blocking other PRs getting merged, I'll approve this and then PR the few accessibility lines myself.

@FreeTubeBot FreeTubeBot merged commit 5230fc4 into FreeTubeApp:development Dec 19, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 19, 2022
@petaded
Copy link
Contributor Author

petaded commented Dec 19, 2022

@ChunkyProgrammer @absidue
Since i couldn't get it working on linux. I have created a windows VM and installed NVDA screenreader on that.

Here is some info to help you get it working properly.
For the reader to work with the remove items for the input-tags element. You need to add the :title="$t('Search Bar.Clear Input') + tag" to the <font-awesome-icon> element and not the span, aria-label as well of course.

Sorry I didn't get this done quick enough to be in this PR. I appreciate your time reviewing PR's

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.

5 participants