Build flexible and accessible accordion component #11087

AllanOXDi · 2023-08-10T20:04:20Z

Summary

This PR creates a flexible and accessible accordion component to support the enhance quiz managment feature .

closes #11011

References

#11011

Reviewer guidance

See the figma design here

PR process

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

github-actions · 2023-08-10T20:37:14Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0b3.dev0_git.124.ga7c85aa0.pex
Windows Installer (EXE)	kolibri-0.16.0b3.dev0+git.124.ga7c85aa0-unsigned.exe
Debian Package	kolibri_0.16.0b3.dev0+git.124.ga7c85aa0-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0b3.dev0+git.124.ga7c85aa0-0.3.0.dmg
Android Package (APK)	kolibri-0.16.0b3.dev0+git.124.ga7c85aa0-0.1.0-debug.apk
TAR file	kolibri-0.16.0b3.dev0+git.124.ga7c85aa0.tar.gz
WHL file	kolibri-0.16.0b3.dev0+git.124.ga7c85aa0-py2.py3-none-any.whl

nucleogenesis

I did some keyboard nav testing and it seems like it's working relatively well, but perhaps we ought to be sure that we're showing the outline on the heading for the AccordionItem.

Currently, the highlight will go from Checkbox to invisible then on to the arrow KIconButton, then to the next row. Looking at the w3 example it appears that the text that would be read by a screen reader ought to also be outlined/highlighted.

Overall - this is excellent work. I know that things get squirrely with the slots and you've got things hooked up very well here. Thank you for your patience working through it all! Look forward to wrapping it up together next week.

(PS I think you need to rebase on develop to resolve the conflicts)

nucleogenesis · 2023-08-18T22:37:25Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/AccordionContainer.vue

+    },
+    data() {
+      return {
+        itemIds: [],


Maybe we ought to call this expandedItemIds to make clear what we're tracking here

nucleogenesis · 2023-08-18T22:38:27Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/AccordionContainer.vue

+        if (this.itemIds.includes(id)) {
+          return true;
+        } else {
+          return false;
+        }


Non-blocking style note - you can also just be return this.itemIds.includes(id) since you're returning a Boolean here.

nucleogenesis · 2023-08-18T22:42:04Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/AccordionContainer.vue

+      isOptionSelected(optionId) {
+        const index = this.itemIds.indexOf(optionId);
+        if (index === -1) {
+          this.optionsIdList.push(optionId);
+        } else {
+          this.optionsIdList.splice(optionId);
+        }
+      },
+      isAnswerSelected(optionId) {
+        if (this.optionsIdList.includes(optionId)) {
+          return true;
+        } else {
+          return false;
+        }
+      },


This kind of logic should live outside of the Accordion altogether. Not all uses of the Accordion will have the check boxes. This is something that will need to be managed by whatever component is making use of the Accordion.

nucleogenesis · 2023-08-18T22:43:51Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

+
+              <template
+                v-if="isItemExpanded(item.id)"
+                #content=""


No need for the ="" here - this is just to indicate what slot the wrapped content will go into in the parent component

nucleogenesis · 2023-08-18T22:46:16Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

+                    <AnswerOption
+                      v-for="answer in item.placeholderAnswers"
+                      :key="answer.id"
+                      :optionId="answer.id"
+                      :isOptionSelected="isOptionSelected"
+                      :isAnswerSelected="isAnswerSelected(answer.id)"
+                    >


I'm sorry I wasn't clear about this - but the question editing business is not necessary here. We won't be doing any kind of question editing in this pass, so this PR just needs to have the accordion component ready to be fleshed out in a follow-up which will wire up everything in #11088 to here, rendering the questions we get from the DB

nucleogenesis

Thanks for the continued efforts here. I've left a few comments about some Kolibri CSS conventions.

I also added some more complex notes around the a11y for the keyboard focus and how we manage clicks, drags, toggles, etc.

nucleogenesis · 2023-08-23T21:43:37Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

+                <KGrid class="question-row">
+                  <KGridItem :layout12="{ span: 6 }">
+                    <div class="left-column-alignment-style">
+                      <DragHandle>


One thing I noticed is that this doesn't show the cursor when hovered over. I think we should probably update the DragHandle component directly so that it has CSS with pointer: cursor so that we get the hand/pointer rather than the normal mouse arrow.

Additionally - the DragHandle doesn't get a focus outline on it when it is focused.

Take a look at kolibri/plugins/learn/assets/src/views/HybridLearningLessonCard.vue LINE 9/10ish for an example of how the $coreOutline styles can be applied to a component.

nucleogenesis · 2023-08-23T21:46:56Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

@@ -223,4 +444,55 @@
    text-align: left;
  }

+  .left-column-alignment-style {
+    display: inline-flex;
+    margin-left: 35px;


We typically try to keep our spacing units in measures of 8px and suggest using em where 1em = 16px as a rule of thumb. So typically we should see values like 0.5em, 1em, 1.5em, etc.

I was looking here because I think the DragHandle should have a bit more space between it and the content to the right of it, so two things to note here:

Convert uses of px to the em value OR to a px value that is a multiple of 8px

Ensure that there is enough space between the DragHandle and the content so that there is no overlap of the keyboard-mode outline

nucleogenesis · 2023-08-23T22:00:38Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

+          <div
+            v-for="item in placeholderList"
+            :key="item.id"
+          >


One thing that came to mind while testing was whether or not keyboard nav focus should cycle through each piece to get to the > KIconButton to open it so I went and tested the W3 example and noticed that the focus flow goes like:

w3accordionkeyboardnav.mp4

However, ours instead of highlighting the whole "header" bit, goes into the children of the row (drag handle -> checkbox -> open/close icon).

Our situation is a bit different because we have interactions within the heading (drag/checkbox) whereas the example above only has text.

This is a little bit of a pickle... because we also want clicking anywhere on the header to toggle open/close and not just the iconbutton - in fact, I think that that KIconButton should really just be a KIcon. BUT the issue might be in that when you click to select a KCheckbox, you'll also end up clicking to trigger the toggle open/close.

For follow up here I think that maybe the next step is to test out what a @click handler here might work okay without getting triggered while clicking the drag handle or KCheckbox. We may need to make some decisions based on how that goes. I'll be happy to cohack a bit on this with you as well to see what we can work out.

nucleogenesis · 2023-08-23T22:01:28Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

+                      <KIconButton
+                        v-if="isItemExpanded(item.id)"
+                        class="icon-size"
+                        icon="chevronUp"
+                        @click="toggleItemState(item.id)"
+                      />
+
+                      <KIconButton
+                        v-else
+                        class="icon-size"
+                        icon="chevronRight"
+                        @click="toggleItemState(item.id)"
+                      />


As noted above - these probably ought to be KIcons assuming we can get everything jiving well w/ focusing/clicking the whole heading element for toggling.

On a separate note I believe KIconButton adds some padding to the icon so you'll probably need to adjust some margin/padding business to align the KIcon here

nucleogenesis · 2023-08-23T22:03:54Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

+
+                  <hr class="bottom-border">
+                  <KButton
+                    style="width:100%;margin-bottom:10px"


My note elsewhere re: typically using 8px increments should also be applied to inline styles in the templates

…s in the accordion panel

radinamatic · 2023-09-01T01:51:18Z

It was a bit difficult for me to comment on a single extracted line of code, but I was able to run this branch in development, and actually test some keyboard navigation 😅

Knowing that this is still work in progress, we could maybe postpone resolving all the green ❔ in the recording that indicate that the Tab key press focused an element somewhere, but there's no indication in the UI what is in focus. More important is the fact that Enter or Space key presses when the dragging button (the first element in each accordion item) is in focus should not open the accordion panel (marked as ❌ in the recording), only Enter or Space key presses on the accordion header button (which gets the correct focus outline, yey!!! 🎉) should result in displaying the content of the panel.

EQ2.mp4

Regarding the missing labels for all the dragging buttons and checkboxes for selection, I already suggested some labels in Slack. This below would be my skeleton version of 2 accordion items, one opened the other closed, sort of a stripped down chimera between the implementation here, and the example on the ARIA APG accordion example. If we add a visually hidden span with additional context, we can use the aria-labelledby which supports the concatenation, to improve the element naming for the screen reader users.

<div class="accordion">

  <button type="button" tabindex="0" class="drag-icon icon-size button" aria-label="Move 'question 1' question"></button>

  <! -- logic for keyboard navigable moving/dragging actions ? --> 

  <input type="checkbox" class="k-checkbox-input" aria-label="Select 'question 1' question">

      <span id=question-title-context class="visuallyhidden">
        Open to review and edit
      </span>

  <h3>
    <button type="button"
            aria-expanded="true"
            class="remove-button-style"
            aria-controls="sect1"
            id="accordion1id"
            aria-labelledby="question-title1 question-title-context">
      <span id=question-title1 class="accordion-title">
        question 1
      </span>

      <svg class="icon-size toggle-icon"></svg>
    </button>
  </h3>

  <div id="sect1"
       aria-labelledby="accordion1id"
       class="accordion-detail-container">

       <! -- content of the question 1 --> 

  </div>

  <button type="button" tabindex="0" class="drag-icon icon-size button" aria-label="Move 'question 2' question"></button>

  <! -- logic for keyboard navigable moving/dragging actions ? --> 

  <input type="checkbox" class="k-checkbox-input" aria-label="Select 'question 2' question">

   <h3>
    <button type="button"
            aria-expanded="false"
            class="remove-button-style"
            aria-controls="sect2"
            id="accordion2id"
            aria-labelledby="question-title2 question-title-context">
      <span id=question-title2 class="accordion-title">
        question 2
      </span>
      <svg class="icon-size toggle-icon"></svg>
    </button>
  </h3>

  <div id="sect2"
       aria-labelledby="accordion2id"
       class="accordion-detail-container">

       <! -- content of the question 2 --> 

  </div>

  <! -- the rest of the questions --> 

</div>

radinamatic · 2023-09-01T02:11:29Z

I would also suggest we disable the dragging with the mouse of accordion items when opened, and enable it only while collapsed. Mostly because questions are bound to have several answers which require panels of certain height, and dragging those elements with a mouse can be a challenge when opened.

radinamatic · 2023-09-01T16:07:29Z

I forgot to add mention for @tomiwaoLE to hear his thoughts about the above ⏫

And I'm also thinking that maybe hearing Open to review and edit is a bit too verbose after every question title... 🤔
Since we already have the string Open in the code base, maybe we could just concatenate that with the title?

tomiwaoLE · 2023-09-07T00:01:23Z

@radinamatic I'm happy with disabling dragging open question accordions, and enabling only while collapsed.

nucleogenesis · 2023-09-12T16:35:45Z

@radinamatic I'm happy with disabling dragging open question accordions, and enabling only while collapsed.

Noting after a chat we've decided to try two approaches:

When clicking the drag-handle of an open item, close the item, then begin dragging.
If that doesn't work, then we will go with just using the up/down icon buttons and will ensure they are accessible.

nucleogenesis · 2023-09-12T16:37:18Z

but there's no indication in the UI what is in focus. More important is the fact that Enter or Space key presses when the dragging button (the first element in each accordion item) is in focus should not open the accordion panel (marked as ❌ in the recording),

Also - one thing to note here is that we should be using the arrows instead of the dragging handle whenever we're in the keyboard modality.

nucleogenesis

@AllanOXDi I've added some thoughts on how possibly to resolve some open issues and clarified some outcomes of discussions re: the drag/sort functionality.

nucleogenesis · 2023-09-12T16:43:07Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

+                              :isLast="index === placeholderList.length - 1"
+                              @moveUp="shiftOne(index, -1)"
+                              @moveDown="shiftOne(index, +1)"
+                              @click.prevent="toggleItemState(item.id)"


I think this is the source of one issue noted by @radinamatic -- this should be triggering the updating of the order of the items, but not toggling the item state. The "toggleItemState" should isntead be on a button that wraps the whole row (I think the button below around line 210 or so).

See the RearrangeChannelsPage for an example; there the @sort event is emitted by the DragContainer where the new order of the items is handled.

Also - I'm having an issue where the drag handle is the only thing being dragged :-/ not sure exactly where the cause of this is by looking at it; but perhaps handling that @sort event will resolve it.

nucleogenesis · 2023-09-12T16:45:06Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

+                        <button
+                          :id="item.id"
+                          :aria-controls="item.id"
+                          aria-expanded="true"


The value here should be :aria-expanded="isItemExpanded(item.id)".

nucleogenesis

Excellent work here Allan. With the feedback above addressed this is ready to merge

github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) SIZE: large labels Aug 10, 2023

AllanOXDi changed the title ~~Add accordion~~ Build flexible and accessible accordion component Aug 10, 2023

github-actions bot added the SIZE: very large label Aug 18, 2023

AllanOXDi assigned nucleogenesis, marcellamaki and thanksameeelian Aug 18, 2023

AllanOXDi marked this pull request as ready for review August 18, 2023 15:15

nucleogenesis requested changes Aug 18, 2023

View reviewed changes

nucleogenesis requested changes Aug 23, 2023

View reviewed changes

AllanOXDi added 10 commits August 31, 2023 00:19

added translatable text

913a6a3

imported components into createquizsection

3bc8675

added draggable component and styling

58c69fe

removed unused styles

2743ce4

removed = on the name of the slot

8f24e68

changed the name of the array to expandedItemIds

9f70212

refactored isItemExpanded method

40e204c

removed the answeroption component

0f8f8d5

added pointer style to the draggable component

fd57f37

changed the px to rem

539de6d

AllanOXDi added 4 commits August 31, 2023 20:22

removed unused methods which were for handling clicking of the option…

11ec9a6

…s in the accordion panel

changed a tag to button with aria-expanded and aria-label

6e0778e

changed the DragContainer to wrap content from the createquizsection

1615439

wrapped the accordionItem into a draggle component

fcf73e9

AllanOXDi requested review from nucleogenesis and marcellamaki August 31, 2023 20:51

removed outline overlapping

75a2f85

wrapped the dragsortwidget

5422bab

disabled dragging when accordion panel is open

5bb2090

rtibbles assigned nucleogenesis Sep 12, 2023

nucleogenesis requested changes Sep 12, 2023

View reviewed changes

AllanOXDi added 5 commits September 13, 2023 16:30

added methods to handle the reorder of a question

4f7cec4

added methods to handle the reorder of a question

0b0cb5d

added to do comment

7927378

disabled the kcheckbox from toggling the accordion panel

9d527dd

saves new list after dragging to the browser

c75db6d

AllanOXDi requested a review from nucleogenesis September 15, 2023 09:20

nucleogenesis mentioned this pull request Sep 19, 2023

Quiz root page with questions available w/ Accordion #11276

Closed

4 tasks

AllanOXDi added 2 commits September 20, 2023 12:39

fixs failing test

9869823

added section_id in utils.spec.js

98c6ab2

nucleogenesis approved these changes Sep 20, 2023

View reviewed changes

AllanOXDi merged commit 17c7368 into learningequality:develop Sep 21, 2023

AllanOXDi deleted the add_accordion branch October 2, 2023 12:08

nucleogenesis mentioned this pull request Jan 11, 2024

Build flexible and accessible accordion component #11011

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build flexible and accessible accordion component #11087

Build flexible and accessible accordion component #11087

AllanOXDi commented Aug 10, 2023 •

edited

Loading

github-actions bot commented Aug 10, 2023 •

edited

Loading

nucleogenesis left a comment

nucleogenesis Aug 18, 2023

nucleogenesis Aug 18, 2023

nucleogenesis Aug 18, 2023

nucleogenesis Aug 18, 2023

nucleogenesis Aug 18, 2023

nucleogenesis left a comment

nucleogenesis Aug 23, 2023

nucleogenesis Aug 23, 2023

nucleogenesis Aug 23, 2023 •

edited by AllanOXDi

Loading

nucleogenesis Aug 23, 2023

nucleogenesis Aug 23, 2023

nucleogenesis Aug 23, 2023

nucleogenesis Aug 23, 2023

radinamatic commented Sep 1, 2023 •

edited

Loading

radinamatic commented Sep 1, 2023 •

edited

Loading

radinamatic commented Sep 1, 2023

tomiwaoLE commented Sep 7, 2023

nucleogenesis commented Sep 12, 2023

nucleogenesis commented Sep 12, 2023

nucleogenesis left a comment

nucleogenesis Sep 12, 2023

nucleogenesis Sep 12, 2023

nucleogenesis Sep 12, 2023

nucleogenesis left a comment

Build flexible and accessible accordion component #11087

Build flexible and accessible accordion component #11087

Conversation

AllanOXDi commented Aug 10, 2023 • edited Loading

Summary

References

Reviewer guidance

PR process

Reviewer checklist

github-actions bot commented Aug 10, 2023 • edited Loading

nucleogenesis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nucleogenesis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nucleogenesis Aug 23, 2023 • edited by AllanOXDi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radinamatic commented Sep 1, 2023 • edited Loading

radinamatic commented Sep 1, 2023 • edited Loading

radinamatic commented Sep 1, 2023

tomiwaoLE commented Sep 7, 2023

nucleogenesis commented Sep 12, 2023

nucleogenesis commented Sep 12, 2023

nucleogenesis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nucleogenesis left a comment

Choose a reason for hiding this comment

AllanOXDi commented Aug 10, 2023 •

edited

Loading

github-actions bot commented Aug 10, 2023 •

edited

Loading

nucleogenesis Aug 23, 2023 •

edited by AllanOXDi

Loading

radinamatic commented Sep 1, 2023 •

edited

Loading

radinamatic commented Sep 1, 2023 •

edited

Loading