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

Applies accessible accordion to SearchKit, adds fallbacks #28441

Merged
merged 10 commits into from Dec 5, 2023

Conversation

vingle
Copy link
Contributor

@vingle vingle commented Dec 3, 2023

Overview

SearchKit builder screen uses two custom angular accordions - this replaces them with <details><summary>.

Before

Search Kit accordions not accessible/keyboard controllable.
image

After

They are.
image

Technical Details

This PR adds some css to stop Bootstrap3 from breaking details/summary, and adds some fallback styling on details/summary as while many modern CMS admin themes will make the summary bold, and change the cursor, older ones don't.

Technical Details

There is a small UI change as the accordion previously used fieldset/legend.

Makes SearchKits Accordion keyboard-navigable, by swapping to details/summary elements
Makes SearchKit's Accordion keyboard-navigable, by swapping to details/summary elements
Primarily needed in SearchKit - but would support any native use of details/summary where the CMS admin theme doesn't already add this kind of CSS.
Copy link

civibot bot commented Dec 3, 2023

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Dec 3, 2023
@ufundo
Copy link
Contributor

ufundo commented Dec 3, 2023

Tested this. HTML markup looks good and keyboard focusing works nicely for me.

But I don't get the open/close icons on default theme. It looks like bootstrap3.css has a rule that is overriding the default display: list-item on summary elements.

image

There seem to be quite a few extra commits as well, could it be rebased?

@ufundo
Copy link
Contributor

ufundo commented Dec 3, 2023

Can see this PR fixes it in Greenwich - but can we not do it below the theme css level?

Something like this in civicrm.css? Bit hacky but it's bootstrap3's fault...

.crm-container #bootstrap-theme summary {
    display: list-item;
}

@vingle
Copy link
Contributor Author

vingle commented Dec 3, 2023

Thanks for testing. This was once one PR but is now separated, so a bit harder to test, sorry.

can we not do it below the theme css level? Something like this in civicrm.css?

civicrm.css would still be overwritten by the theme (so the themes would still need updating). That's why this started outside of the theme path, at ext/search_kit/css/crmSearchAdmin.css - but that only applies to this page, and <details><summary> will likely appear elsewhere.

@artfulrobot
Copy link
Contributor

Tested, is an improvement. I think the test fails are not related.

@artfulrobot
Copy link
Contributor

Jenkins, test this please

@colemanw
Copy link
Member

colemanw commented Dec 5, 2023

retest this please

@artfulrobot
Copy link
Contributor

@vingle are the conversations with @ufundo resolved? I wasn't sure from the above? Should this be merged? tests are passing (though I expect none of this is tested.)

@vingle
Copy link
Contributor Author

vingle commented Dec 5, 2023

@artfulrobot - well he hasn't replied but didn't bring it up at the sprint - and the point he raises was discussed by Coleman and I - and I think this is the least worst way to go given the various dependencies.

That said, I have a new PR for civicrm.css almost ready around accordions, so maybe it makes sense to remove civicrm.css from this PR and make it just about SearchKit? And have a separate PR for the proposed change to civicrm.css so it's all together (and easier to test once / replicate in a theme)?

@artfulrobot artfulrobot merged commit 96ff3c8 into civicrm:master Dec 5, 2023
3 checks passed
@artfulrobot
Copy link
Contributor

I've merged for the sake of moving things forward! Thanks @vingle!

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