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: add filters for gt labelling and gt script-type #88

Merged
merged 30 commits into from
Jul 26, 2024
Merged

Conversation

jfrer
Copy link
Collaborator

@jfrer jfrer commented Jun 28, 2024

closes #68

@jfrer jfrer requested review from t11r July 2, 2024 13:58
@t11r
Copy link
Collaborator

t11r commented Jul 9, 2024

The code itself looks fine, but there a some accessibility concerns:

  • All controls should be labelled, and while the checkbox for toggling all filters has an ARIA attribute, a label legible by everyone would not hurt.
  • There are many contrast errors, but only few of those have been introduced by this PR.

I would suggest to check with the WAVE browser extension.

Also, lots of Vue warnings are logged to the console about missing props, extraneous non-props attributes and missing translations. This should be investigated.

@jfrer
Copy link
Collaborator Author

jfrer commented Jul 12, 2024

The code itself looks fine, but there a some accessibility concerns:

  • All controls should be labelled, and while the checkbox for toggling all filters has an ARIA attribute, a label legible by everyone would not hurt.
  • There are many contrast errors, but only few of those have been introduced by this PR.

I would suggest to check with the WAVE browser extension.

Also, lots of Vue warnings are logged to the console about missing props, extraneous non-props attributes and missing translations. This should be investigated.

Thanks for the extensive review.

  • I added a label to the checkbox for toggling all filters in 9639690. As there is no native functionality/slot for this in the PrimeVue component we use, I had to implement it manually. In this move I implemented a wrapper component to avoid/reduce code duplication.

  • The WAVE Tool seems very nice, but I can't find contrast errors that have been introduced by this PR. Can you specify where you found these?
    I will resolve the rest of the contrast errors in a seperate PR.

  • I also noticed the Vue warnings when I started this project a month ago and got so annoyed by the amount of warnings that I deselected the warnings filter in the console. 😅 I will also try to fix them in a seperate PR.

@t11r
Copy link
Collaborator

t11r commented Jul 12, 2024

The WAVE Tool seems very nice, but I can't find contrast errors that have been introduced by this PR. Can you specify where you found these?

Selected items in the dropdowns have a contrast ratio of 2.55 or lower, while WCAG AA requires at least 4.5 for normal-sized text. In general, the blue is too light, especially on a colored background.

@jfrer
Copy link
Collaborator Author

jfrer commented Jul 19, 2024

Selected items in the dropdowns have a contrast ratio of 2.55 or lower, while WCAG AA requires at least 4.5 for normal-sized text. In general, the blue is too light, especially on a colored background.

Ah, thanks, I see. As we didn't put much thought into coloring yet, these are just default color stylings from the lara-light-blue theme. I will change the theme to a more accessible one when I will work on #91, because I think that a color theme change is a little bit out of scope of this PR. For now, I made the coloring of the "Select all"-Label a variable that will change with the theming.

@jfrer jfrer merged commit 65da64d into master Jul 26, 2024
1 check passed
@jfrer jfrer deleted the gt-filtering branch July 26, 2024 09:43
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.

Quiver timeline - add GT filters
4 participants