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

[#78] Fixed Single and Group filter announcing changes to content and a "down" key trigger for collapsible. #118

Merged
merged 4 commits into from
Apr 10, 2024

Conversation

joshua-salsadigital
Copy link
Collaborator

@joshua-salsadigital joshua-salsadigital commented Mar 21, 2024

Closes #78

Checklist before requesting a review

  • I have formatted the subject to include the issue number
    as [#123] Verb in past tense with a period at the end.
  • I have provided information in the Changed section about WHY something was
    done if this was a bespoke implementation.
  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run new and existing relevant tests locally with my changes,
    and they have passed.
  • I have provided screenshots, where applicable.

Changed

  1. Fixed: The filters and options are read by a screenreader left to right, top to bottom (e.g. first row left to right and then second row left to right).
  2. Fixed: The focus is visible on each select box in drop down when navigating using a mouse or keyboard.
  3. Fixed: The user can tab to each interactive element and interact with the filter using keyboard or mouse.
  4. Fixed: If the contents underneath the filter change when an option is selected, the change should be announced using aria-live=”polite” - FAIL.

Screenshots

…o right, top to bottom (e.g. first row left to right and then second row left to right).
@joshua-salsadigital joshua-salsadigital added the State: Needs more work The issue requires more work label Mar 21, 2024
@joshua-salsadigital joshua-salsadigital self-assigned this Mar 21, 2024
Copy link

github-actions bot commented Mar 21, 2024

@github-actions github-actions bot temporarily deployed to pull request March 21, 2024 05:29 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 21, 2024 06:46 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 25, 2024 09:20 Inactive
@AlexSkrypnyk AlexSkrypnyk added PR: Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request and removed State: Needs more work The issue requires more work labels Mar 25, 2024
@joshua-salsadigital joshua-salsadigital added PR: Needs review Pull request needs a review from assigned developers and removed PR: Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request labels Mar 26, 2024
Copy link
Contributor

@AlexSkrypnyk AlexSkrypnyk left a comment

Choose a reason for hiding this comment

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

@joshua-salsadigital
please see my inline comments

t.el.setAttribute('aria-live', 'polite');
};

// Find group Filter components and initialize
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this commnet

* Update the instance.
*/
CivicThemeGroupFilterComponent.prototype.update = function () {
const t = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

use this directly


this.el = el;

this.el.addEventListener('ct.group-filter.update', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this.el.addEventListener('ct.group-filter.update',  this.update.bind(this, true)

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Function/bind#syntax

this.update(true);
});

// Function to trigger custom event
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this and other comments like this - they are not describing anything useful

};

// Function to bind event listeners
const bindEventListeners = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

please simplify this by not using the bindEventListeners

this.el.querySelectorAll('input, textarea, select, [type="checkbox"], [type="radio"]').forEach((input) => {
      input.addEventListener('change', function(){
         // Find parent.
         const parent = this.parent..
         parent.dispatchEvent(new CustomEvent('ct.group-filter.update'));      
      });
    });

* @file
* Single Filter component.
*/
function CivicThemeSingleFilterComponent(el) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to have the same updates as above

@AlexSkrypnyk AlexSkrypnyk added PR: Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request and removed PR: Needs review Pull request needs a review from assigned developers labels Mar 26, 2024
@joshua-salsadigital joshua-salsadigital added PR: Needs review Pull request needs a review from assigned developers and removed PR: Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request labels Apr 3, 2024
@github-actions github-actions bot temporarily deployed to pull request April 3, 2024 16:53 Inactive
@AlexSkrypnyk AlexSkrypnyk changed the title [#78] Accessibility fails on List component [#78] Fixed Single and Group filter announcing changes to content and a "down" key trigger for collapsible. Apr 10, 2024
@AlexSkrypnyk AlexSkrypnyk enabled auto-merge (squash) April 10, 2024 05:32
@AlexSkrypnyk AlexSkrypnyk disabled auto-merge April 10, 2024 05:34
@AlexSkrypnyk AlexSkrypnyk merged commit ef0e63a into main Apr 10, 2024
4 checks passed
@AlexSkrypnyk AlexSkrypnyk deleted the feature/78-group-filter-ally-fixes branch April 10, 2024 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Needs review Pull request needs a review from assigned developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility fails on List component
2 participants