dev/user-interface#60 Fixes UI regression & makes more accordions accessible #29533

vingle · 2024-02-28T23:11:19Z

While testing @aydun's #29447, I discovered a CSS-based UI regression from the CSS changes in 5.69: accordions using the .crm-collapsible pattern (of which I've found 9) don't change the expand/close caret icon on open close. The accordions still work, but the triangle is set to open position, even when the accordion is closed.

Rather than just fix this with CSS, I figured this was a good chance to replace the pattern with the more accessible markup pattern. Between this and Aidan's PR, this would only leave a handful of non-accessible accordions left.

Overview

I've gone thru all but one of the instances of .crm-collapsible for accordions and replaced with <details><summary> using .crm-accordion-light where appropriate to try to match the original UI. Have tested each, and checked for related JS in the template files that might now error out.

Before

After

Technical details

I've added container <div class="crm-accordion-body"> where that didn't exist. In one instance I've moved a region that was ins

Comments

Testing template files can take a bit of detective work to find them in the UI, so here's the paths for each:

templates/CRM/Admin/Form/ScheduleReminders.tpl - civicrm/admin/scheduleReminders?reset=1 & click 'add reminder'.
templates/CRM/Admin/Page/APIExplorer.tpl - civicrm/api3 & then select an Entity, such as ACLRole.
templates/CRM/Case/Form/ActivityTab.tpl - civicrm/contact/view/case?reset=1 under Activities/SearchFilters (might need a case) - NB despite all the red/green, I just changed two different accordion patterns here, added a body wrapper and adjusted the tabbing, but the diff looks much bigger than that
templates/CRM/Contact/Form/Edit/Address/CustomData.tpl - civicrm/contact/add?reset=1 + the address fields need custom data
templates/CRM/Contact/Form/Edit/Email.tpl - civicrm/contact/add?reset=1 the primary email (after save) should have an accordion to add a signature.
templates/CRM/Contribute/Form/ManagePremiums.tpl - civicrm/admin/contribute/managePremiums?reset=1 + click 'add premium'.
templates/CRM/Event/Form/ManageEvent/Registration.tpl - civicrm/event/manage/settings?reset=1&action=update&id=2&selectedChild=registration - the large diff here is down to wrapping a chunk in a crm-accordion-body div.
templates/CRM/Financial/Form/FinancialBatch.tpl - civicrm/financial/batch?reset=1&action=add

…rd accessible

civibot · 2024-02-28T23:11:21Z

🤖 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...

➡️ PR Demo site (can be rebuilt with the command test this please).
📖 Review standards
🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

civibot · 2024-02-28T23:11:24Z

No issue was found matching the issue reference given in the pull request title. Please check the syntax.

highfalutin · 2024-02-29T15:25:23Z

Currently reviewing; tracking manual testing at https://lab.civicrm.org/dev/user-interface/-/issues/67#testing-29533

vingle · 2024-02-29T16:41:13Z

One note when reviewing this: @colemanw fixed one of these last week (#29446) and decided in the end it didn't need to be an accordion at all as so little was revealed – that's an alternative path with these.

shaneonabike · 2024-02-29T19:16:37Z

Hi Nicol,

Quick question: I noticed that for custom_groups in Event Registration it is not converting the pre-existing divs to a details/summary combo. I tried manually changing it and things work. I just wanted to checkin about that (and perhaps it's just in another PR?)

cc @vingle

vingle · 2024-02-29T19:48:56Z

@shaneonabike - If I remember right, I think custom fields are being handled in #29448 in these files:

shaneonabike · 2024-02-29T20:45:04Z

Good to go from Wordpress perspective extra for the extra class you added causing theming issues. If you fix that I'm good on this point to merge.

cc @vingle

highfalutin · 2024-02-29T20:48:28Z

I reviewed the first three changes at https://lab.civicrm.org/dev/user-interface/-/issues/67#testing-29533. The API v3 Explorer change breaks the interface, so IMO we should remove that one. The Case View one creates some extra (kind of ugly) borders around the detail content. I'll try to make a commit that fixes these.

vingle · 2024-02-29T23:43:28Z

Thanks alot @highfalutin - I hadn't seen the API3 issues.

The css border was on my radar and I was waiting for the accordions to be merged before putting in a css tidy-up. But there's no harm in making that PR now: #29551. It fixes both the border that is mistakenly inherited from .crm-accordion-bold and adds a tiny bit of breathing space between multiple accordions, similar to how it was previously.

vingle · 2024-03-01T00:13:25Z

Good to go from Wordpress perspective extra for the extra class you added causing theming issues. If you fix that I'm good on this point to merge.

@shaneonabike - are you able to give a bit more details/screengrab? The new classes are in civicrm.css, which WordPress loads, is this specific to CAU? Quite a few of the new classes have been added there by Christian.

highfalutin · 2024-03-01T14:21:43Z

@vingle @herbdool thanks for fixing the border and spacing issues! I'm about to respond to @vingle's feedback on my PR 13.

…n Case View

highfalutin · 2024-03-01T14:42:55Z

So at this point, I've reviewed and approved/submitted fixes for the first 3 files; Shane has reviewed and approved the rest. Once Nic merges my changes (13), this PR can be merged.

templates/CRM/Contact/Form/Edit/Email.tpl

shaneonabike · 2024-03-01T16:56:09Z

Good to go from Wordpress perspective extra for the extra class you added causing theming issues. If you fix that I'm good on this point to merge.

@shaneonabike - are you able to give a bit more details/screengrab? The new classes are in civicrm.css, which WordPress loads, is this specific to CAU? Quite a few of the new classes have been added there by Christian.

No I wrote a review point but it was still in pending -- I didn't realize I needed to submit it - 😱 . Anyway you'll need the slight change. Other than that you are totally good from my testing on WP.

cc @vingle

revert changes to API v3 explorer, remove redundant class on Case View

vingle · 2024-03-01T17:31:03Z

Thanks @highfalutin, @shaneonabike & @herbdool – the API3 fix is merged, and the email signature issue should be fixed when #29448 (or the part of it relating to the contact page, which has passed tests) is merged.

highfalutin · 2024-03-01T18:04:38Z

@colemanw test failures are unrelated since this pr only touches . tpl files

colemanw · 2024-03-01T18:16:08Z

templates/CRM/Admin/Form/ScheduleReminders.tpl

@@ -144,11 +144,12 @@
            </tr>
          </table>
            {include file="CRM/Contact/Form/Task/EmailCommon.tpl" upload=1 noAttach=1}
-        </div>
-      </fieldset>
+        </div><!-- /.crm-accordion-body -->


<nitpick>
I don't mean to be a pain... but I really don't like these comments. It's 2024 and we all know what a closing tag is.
</nitpick> 

Yes, especially as it sits next to the closing details tag (or should be). Perhaps at the end of this there can be a civi wide cleanup of  & `?

Sure. Ideally this PR wouldn't be adding new ones that we then have to cleanup.
But if that's easier then ok I can merge this.

Those comments are now removed for all the templates in #29448

fixes ui regression and makes accordions using crm-collapsible keyboa…

735b4e2

…rd accessible

civibot bot added the master label Feb 28, 2024

vingle mentioned this pull request Feb 29, 2024

Convert accordions to accessible style #29448

Merged

shaneonabike mentioned this pull request Feb 29, 2024

Convert more accordions #29447

Closed

revert changes to API v3 explorer, remove redundant class on Case View

5e525e2

vingle mentioned this pull request Feb 29, 2024

Fixes to Accordion CSS, following recent PRs #29551

Merged

remove old accordion pattern from API3 explorer, restore class name o…

20c5658

…n Case View

highfalutin added the almost merge on pass label Mar 1, 2024

shaneonabike suggested changes Mar 1, 2024

View reviewed changes

templates/CRM/Contact/Form/Edit/Email.tpl Show resolved Hide resolved

vingle added 2 commits March 1, 2024 17:13

Merge pull request #13 from lemniscus/accordion-collapsible

9ac9cbc

revert changes to API v3 explorer, remove redundant class on Case View

Merge branch 'civicrm:master' into accordion-collapsible

777ebc7

highfalutin added merge on pass and removed almost merge on pass labels Mar 1, 2024

colemanw reviewed Mar 1, 2024

View reviewed changes

shaneonabike mentioned this pull request Mar 1, 2024

Fixes to Accordion Helper Icon CSS, following recent PRs #29551 #29563

Merged

colemanw merged commit be79fbb into civicrm:master Mar 1, 2024
2 of 3 checks passed

vingle mentioned this pull request Mar 2, 2024

Cleans up some old accordion classnames still being used with details/summary #29600

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/user-interface#60 Fixes UI regression & makes more accordions accessible #29533

dev/user-interface#60 Fixes UI regression & makes more accordions accessible #29533

vingle commented Feb 28, 2024

civibot bot commented Feb 28, 2024

civibot bot commented Feb 28, 2024

highfalutin commented Feb 29, 2024

vingle commented Feb 29, 2024

shaneonabike commented Feb 29, 2024

vingle commented Feb 29, 2024

shaneonabike commented Feb 29, 2024

highfalutin commented Feb 29, 2024

vingle commented Feb 29, 2024

vingle commented Mar 1, 2024

highfalutin commented Mar 1, 2024

highfalutin commented Mar 1, 2024

shaneonabike commented Mar 1, 2024

vingle commented Mar 1, 2024

highfalutin commented Mar 1, 2024

colemanw Mar 1, 2024

vingle Mar 1, 2024

colemanw Mar 1, 2024

aydun Mar 2, 2024

dev/user-interface#60 Fixes UI regression & makes more accordions accessible #29533

dev/user-interface#60 Fixes UI regression & makes more accordions accessible #29533

Conversation

vingle commented Feb 28, 2024

Overview

Before

After

Technical details

Comments

civibot bot commented Feb 28, 2024

civibot bot commented Feb 28, 2024

highfalutin commented Feb 29, 2024

vingle commented Feb 29, 2024

shaneonabike commented Feb 29, 2024

vingle commented Feb 29, 2024

shaneonabike commented Feb 29, 2024

highfalutin commented Feb 29, 2024

vingle commented Feb 29, 2024

vingle commented Mar 1, 2024

highfalutin commented Mar 1, 2024

highfalutin commented Mar 1, 2024

shaneonabike commented Mar 1, 2024

vingle commented Mar 1, 2024

highfalutin commented Mar 1, 2024

colemanw Mar 1, 2024

Choose a reason for hiding this comment

vingle Mar 1, 2024

Choose a reason for hiding this comment

colemanw Mar 1, 2024

Choose a reason for hiding this comment

aydun Mar 2, 2024

Choose a reason for hiding this comment