Page MenuHomePhabricator

Design and build Accordion component (MVP)
Closed, ResolvedPublic

Assigned To
Authored By
bmartinezcalvo
Jan 10 2023, 6:12 PM
Referenced Files
F37121175: Captura de pantalla 2023-06-27 a las 19.21.37.png
Jun 27 2023, 5:23 PM
F37121167: Screenshot 2023-06-27 at 19.02.13.png
Jun 27 2023, 5:05 PM
F37121131: Captura de pantalla 2023-06-27 a las 18.17.15.png
Jun 27 2023, 4:23 PM
F37121121: Captura de pantalla 2023-06-27 a las 18.14.01.png
Jun 27 2023, 4:23 PM
F37121119: Captura de pantalla 2023-06-27 a las 18.11.47.png
Jun 27 2023, 4:23 PM
F37119788: Captura de pantalla 2023-06-26 a las 11.46.35.png
Jun 26 2023, 9:51 AM
F37119786: Captura de pantalla 2023-06-26 a las 11.45.41.png
Jun 26 2023, 9:51 AM
F37113571: Captura de pantalla 2023-06-23 a las 11.31.55.png
Jun 23 2023, 9:32 AM
Tokens
"Party Time" token, awarded by CCiufo-WMF.

Description

This task defines the minimum viable product (MVP) of the Accordion component.


Scope

The MVP covers these Abstract Wikipedia use cases:

Wikifunctions T323013
Captura de Pantalla 2022-12-16 a las 12.04.46.png (1×600 px, 355 KB)

Also the accordion is being used in Web Improvements:

Vector Tools panel (Prototype)
Captura de Pantalla 2022-12-16 a las 12.24.13.png (1×1 px, 730 KB)
Expanded
Captura de Pantalla 2022-12-16 a las 12.24.20.png (1×1 px, 713 KB)

Considerations

To be added.

Open questions

  • End action: currently it’s visible just when the accordion is expanded. Do we want to maintain it in the label section or could we move it to the accordion content? End action will be always visible on the head section. In some cases, it will be visible as default and in other cases it will be visible just when expanded (e.g. Minerva's article). Check the recommendations section.
    Captura de Pantalla 2023-02-01 a las 17.48.05.png (1×1 px, 1 MB)
  • Arrow position: Op.1. Up-Down / Op.2. Right-Down We will go with the Up-Down arrow for this MVP task and we will iterate in the future if needed.
    Captura de Pantalla 2023-02-01 a las 17.48.48.png (802×1 px, 259 KB)
  • Contour properties: the default version of the component will include dividers to separate each stacked accordion. Do we want to include the version without dividers or the version with outline? We will implement just the accordion with dividers for this MVP task and we will include the rest of props in the future if needed.
    Captura de Pantalla 2023-02-01 a las 17.49.45.png (982×2 px, 513 KB)

Design spec

Anatomy
  • Head with arrow icon and label that can be expanded
  • Body with free content that is expanded when opening the head
Style
  • Arrow icon: 20px (this icon cannot be customizable)
  • Label: can be customizable
  • End action: quiet icon-only button (if needed)
  • Expandable content: customizable (e.g. text paragraphs, menu items, links, images, table) (some examples will be included in the demo)

*Accordions will be vertically stacked (free number of accordion items).

Interaction
  • Collapsed: just the label is visible (Default, Hover, Active, Focus, Disabled)
  • Expanded: label and accordion content are visible (Default, Hover, Active, Focus) The different states changes will be visible just in the head section.
NOTE: the arrow will rotate when expands/collapses. We will apply our system transition tokens to expand and collapse the expanded section.
Documentation

Configurable demo will include:

  • Description: use a slot control to add/remove the description text
  • End action prop (icon-only button) with customizable icon if possible
  • Disabled: enable/disable the accordion with a toggle switch
  • Slot to customize the text in the body content and make it longer if necessary
  • LTR/RTL visualization

Other demos in the page will include:

  • Customizable label's text style (customize with one of our text tokens, e.g. Heading L 24/30 used in the Minerva's Wikipedia article)
  • Represent other body content within the accordion (e.g. an image, a combination of form items, etc.)

Acceptance criteria

  • A Figma spec sheet is created for the component that includes the scope defined here. A link to the Figma spec sheet's MVP version has been added to this task's description.
  • The initial component is implemented in Codex.

Design review

More info in T326665#8905947

  • The label in the accordion should use the base text that in Codex is 16/22
  • Could we extend the accordion content to a bigger paragraph? We could use this text from the spec "The accordion content can be as longer as needed, and the type of content can vary according to the need of each use case. Headings should be used to label each section of content."
  • Padding between the arrow and the accordion label should be 8px @spacing-50
  • Accordion content - padding on top should be 8px @spacing-50
  • Remove hover from disabled state
  • Align the end action with the accordion's title and arrow
  • Use the same text size for accordion's title and body content in the demos
  • Updated the configurable demo including:
    • Description: use a slot control to add/remove the description text
    • Leave the configurable demo without visible description as default (the user will be able to type the description in the slot if needed)
    • End action prop (icon-only button) with customizable icon if possible
    • Disabled: enable/disable the accordion with a toggle switch
    • Slot with a text input to type and customize the text in the accordion's body content to make it longer/shorter
    • LTR/RTL visualization

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Good question @CCiufo-WMF and thanks for laying out the options @egardner - I would recommend we update the markup of the Vue component to use <details> (in a future patch) and use that for the CSS-only version as well. I can't get behind the idea of not using the appropriate, semantic HTML element just to support IE11

I agree we should leave it to a follow up task. We should also consider whether or not CSS-only components should be considered part of an MVP component, or if they should be "planned component updates" that are created at time of MVP component implementation. Is there a task for this already? If not I can create one!

I've included the component's documentation in the task description explaining how we could represent the accordion in the configurable demo and other demos.

Change 921672 merged by jenkins-bot:

[design/codex@main] Accordion: initial implementation of accordion component

https://gerrit.wikimedia.org/r/921672

The initial implementation is now merged (as a WIP component), and can be viewed here. It's ready for an initial design review.

Based on a quick comparison between the spec sheet and the implementation, here's what I think is still missing:

  • Add an optional description below the title (this is shown in the spec sheet, but not implemented)
  • Implement correct behavior for long titles
    • Titles should wrap, but they currently don't
    • Titles should not go under the end icon, but they currently do (we need to add padding on the title to make room for the absolutely positioned end icon)
  • Revisit padding/margin on the content
    • Right now there's 12px padding around the content, but the Figma spec sheet appears to call for 8px on the top (but still 12px on the bottom and on the left+right)

I agree we should leave it to a follow up task. We should also consider whether or not CSS-only components should be considered part of an MVP component, or if they should be "planned component updates" that are created at time of MVP component implementation. Is there a task for this already? If not I can create one!

I have created T338184 for this.

Change 927313 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[design/codex@main] Accordion: Add ARIA-labels for icon-only buttons

https://gerrit.wikimedia.org/r/927313

Change 927313 merged by jenkins-bot:

[design/codex@main] Accordion: Add ARIA-labels for icon-only buttons

https://gerrit.wikimedia.org/r/927313

Design sign-off done. Adding some things we should fix:

  1. The label in the accordion should use the base text that in Codex is 16/22. Updated the spec to reflect this.
  2. Could we extend the accordion content to a bigger paragraph? We could use this text from the spec "The accordion content can be as longer as needed, and the type of content can vary according to the need of each use case. Headings should be used to label each section of content."
  3. Padding between the arrow and the accordion label should be 8px @spacing-50
  4. Accordion content - padding on top should be 8px @spacing-50 (now it's 12px so it makes the accordion content too separated from the label)

Apart form this, I added some notes in the task description about the configurable and standalone demos, could we update them?

Configurable demo should include:

  • Description: hide/display it with a toggle switch
  • End action prop (icon-only button) with customizable icon if possible
  • Disabled: enable/disable the accordion with a toggle switch
  • Slot to customize the text in the body content and make it longer if necessary
  • LTR/RTL visualization

Other demos in the page should include:

  • Customizable label's text style (customize with one of our text tokens, e.g. Heading L 24/30 used in the Minerva's Wikipedia article)
  • Represent other body content within the accordion (e.g. an image, a combination of form items, etc.)

Moving the task to Ready for development so we can fix the list commented above.

bmartinezcalvo updated the task description. (Show Details)
bmartinezcalvo updated the task description. (Show Details)

Change 927786 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[mediawiki/core@master] Update Codex from v0.11.0 to v0.12.0

https://gerrit.wikimedia.org/r/927786

Test wiki created on Patch demo by ATomasevich (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/8ca78d2715/w

Change 927786 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v0.11.0 to v0.12.0

https://gerrit.wikimedia.org/r/927786

Test wiki on Patch demo by ATomasevich (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/8ca78d2715/w/

Hey @Sswiergosz, the Design Systems team is wondering if you have the capacity to make a few last updates that came out of the design review for this component (see the updated task description). Once those are complete we can publish the component!

Hi @CCiufo-WMF, sorry for the recent inactivity but I've been on sick leave for the last two weeks. I can work on it in the first half of next week. Does it suit you?

Hi @CCiufo-WMF, sorry for the recent inactivity but I've been on sick leave for the last two weeks. I can work on it in the first half of next week. Does it suit you?

I'm sorry to hear that, I hope you are feeling better. There's no urgency around this on our end -- just wanted to check in to see if you were still interested in contributing post-hackathon. Next week is perfectly fine!

Thanks, I'm much better now! I'll definitely take care of it next week. Besides the design review changes, I see some other things also changed (e.g. padding around action buttons) and I'll revisit it as well.

Also, I'm not sure why tabindex property was added to the component and set to -1. Because of that, it's impossible to interact with the accordion using a keyboard. @egardner I see that you added tabindex. Could you explain why? Thanks!

Design sign-off done. Adding some things we should fix:

  1. The label in the accordion should use the base text that in Codex is 16/22. Updated the spec to reflect this.
  2. Could we extend the accordion content to a bigger paragraph? We could use this text from the spec "The accordion content can be as longer as needed, and the type of content can vary according to the need of each use case. Headings should be used to label each section of content."
  3. Padding between the arrow and the accordion label should be 8px @spacing-50
  4. Accordion content - padding on top should be 8px @spacing-50 (now it's 12px so it makes the accordion content too separated from the label)

Apart form this, I added some notes in the task description about the configurable and standalone demos, could we update them?

Configurable demo should include:

  • Description: hide/display it with a toggle switch
  • End action prop (icon-only button) with customizable icon if possible
  • Disabled: enable/disable the accordion with a toggle switch
  • Slot to customize the text in the body content and make it longer if necessary
  • LTR/RTL visualization

Other demos in the page should include:

  • Customizable label's text style (customize with one of our text tokens, e.g. Heading L 24/30 used in the Minerva's Wikipedia article)
  • Represent other body content within the accordion (e.g. an image, a combination of form items, etc.)

Moving the task to Ready for development so we can fix the list commented above.

Hi @bmartinezcalvo - could you please explain what do you mean by Customizable label's text style (customize with one of our text tokens, e.g. Heading L 24/30 used in the Minerva's Wikipedia article) ?

I'm not familiar with this text token for heading :)

Design sign-off done. Adding some things we should fix:

  1. The label in the accordion should use the base text that in Codex is 16/22. Updated the spec to reflect this.
  2. Could we extend the accordion content to a bigger paragraph? We could use this text from the spec "The accordion content can be as longer as needed, and the type of content can vary according to the need of each use case. Headings should be used to label each section of content."
  3. Padding between the arrow and the accordion label should be 8px @spacing-50
  4. Accordion content - padding on top should be 8px @spacing-50 (now it's 12px so it makes the accordion content too separated from the label)

Apart form this, I added some notes in the task description about the configurable and standalone demos, could we update them?

Configurable demo should include:

  • Description: hide/display it with a toggle switch
  • End action prop (icon-only button) with customizable icon if possible
  • Disabled: enable/disable the accordion with a toggle switch
  • Slot to customize the text in the body content and make it longer if necessary
  • LTR/RTL visualization

Other demos in the page should include:

  • Customizable label's text style (customize with one of our text tokens, e.g. Heading L 24/30 used in the Minerva's Wikipedia article)
  • Represent other body content within the accordion (e.g. an image, a combination of form items, etc.)

Moving the task to Ready for development so we can fix the list commented above.

Hi @bmartinezcalvo - could you please explain what do you mean by Customizable label's text style (customize with one of our text tokens, e.g. Heading L 24/30 used in the Minerva's Wikipedia article) ?

I'm not familiar with this text token for heading :)

@Sswiergosz I meant the accordion label text will use the base text style documented in the spec (Bold) but it could be customized with another text style (using any of our Font tokens) to cover other use cases. Anyway, the last two bullets in my list will be finally done in this other task T338821 so you don't need to complete them now.

Captura de pantalla 2023-06-23 a las 11.31.55.png (1×2 px, 215 KB)

Change 932412 had a related patch set uploaded (by Sswiergosz; author: Sswiergosz):

[design/codex@main] Accordion: changes after design review

https://gerrit.wikimedia.org/r/932412

Thanks @bmartinezcalvo. I submitted the patch without font tokens for now. I updated the docs to show different body content within the accordion though.

I fixed all of the comments after the Design Review. I also added functionality to wrap too-long titles/descriptions to the next lines.

Thanks @bmartinezcalvo. I submitted the patch without font tokens for now. I updated the docs to show different body content within the accordion though.

I fixed all of the comments after the Design Review. I also added functionality to wrap too-long titles/descriptions to the next lines.

@Sswiergosz thank you! I've reviewed the new patch and all comments have been fixed except the one related with the customizable demo, including it here:

  • Updated the configurable demo including:
    • Description: hide/display it with a toggle switch
    • End action prop (icon-only button) with customizable icon if possible
    • Disabled: enable/disable the accordion with a toggle switch
    • Slot with a text input to type and customize the text in the accordion's body content to make it longer/shorter
    • LTR/RTL visualization

Is it possible to include these items in the configurable demo (the first demo in the page)? We always include a configurable demo in each component's page (e.g. Button) so he user can play with the different properties of the component. So, in this case, the Default demo could be customizable and renamed it to "Configurable demo"

Captura de pantalla 2023-06-26 a las 11.45.41.png (1×1 px, 133 KB)

Apart from this, one small thing I would update is the demo with visible icon:

  1. I would rename it to "With always visible icon action" since the action is not an icon but a button instead.
  2. Although the button's icon can be customized to any action, I would include in this example all edit buttons since, to represent more reality, we will usually have the same actions in the same accordion group.

Captura de pantalla 2023-06-26 a las 11.46.35.png (776×1 px, 86 KB)

@bmartinezcalvo sure thing, I can include that configurable demo! I wasn't sure at the beginning what do you mean by that but the examples helped a lot :) I have only one question. You wrote "Description: hide/display it with a toggle switch" but currently the description is passed using a slot. Why do we need such a switch for toggling the visibility of a description? I'm unsure if the switch is needed; no prop in the code handles that.

@Sswiergosz I agree; since the description is a slot, we don't really have a mechanism in the configurable demo system for turning it on or off with a switch. Instead, you can just include it as a slot control (see the configurable demo for the Field component for an example with multiple slots) and people can delete the text there to see the component without a description.

@Sswiergosz I agree; since the description is a slot, we don't really have a mechanism in the configurable demo system for turning it on or off with a switch. Instead, you can just include it as a slot control (see the configurable demo for the Field component for an example with multiple slots) and people can delete the text there to see the component without a description.

@Sswiergosz yes, you can solve it with the lot control instead as Anne commented. Updated the task description with it. Thank you!

I included the configurable demo. All requirements should be addressed now. Also, I added aria-disabled property to the accordion, somehow I missed that during the development.

I included the configurable demo. All requirements should be addressed now. Also, I added aria-disabled property to the accordion, somehow I missed that during the development.

Thank you @Sswiergosz! The items in the design review list have been checked. But I've found the following things to fix while reviewing the new configurable demo:

  1. Disabled state should not receive hover
  2. Since the base case for the accordion will be without a description, I would leave the accordion in the demo without any description (then, the user will be able to type the description in the slot if needed)
    Captura de pantalla 2023-06-27 a las 18.11.47.png (1×1 px, 131 KB)
  3. End action should be aligned with the accordion's title and arrow (now the action is centered in the height)
    Captura de pantalla 2023-06-27 a las 18.14.01.png (416×1 px, 28 KB)
  4. Accordion's content: I know it can be customized with any content or text size, but it seems weird to view this body as smaller than the title and description. Could we use the same text size for all of them in the demos?

Captura de pantalla 2023-06-27 a las 18.17.15.png (276×1 px, 40 KB)

I'm not sure about 3rd point - the end icon is centered in the height but its container has the same height as the title and arrow container and every of that element is centered vertically. Are you sure it's not aligned? Maybe I'm missing something here @bmartinezcalvo

Screenshot 2023-06-27 at 19.02.13.png (552×1 px, 89 KB)

I'm not sure about 3rd point - the end icon is centered in the height but its container has the same height as the title and arrow container and every of that element is centered vertically. Are you sure it's not aligned? Maybe I'm missing something here @bmartinezcalvo

Screenshot 2023-06-27 at 19.02.13.png (552×1 px, 89 KB)

@Sswiergosz the problem with the icon alignment is when the description is displayed.

Captura de pantalla 2023-06-27 a las 19.21.37.png (140×954 px, 30 KB)
Captura de pantalla 2023-06-27 a las 18.14.01.png (416×1 px, 28 KB)
Current demoHow it should be

Change 932412 merged by jenkins-bot:

[design/codex@main] Accordion: changes after design review

https://gerrit.wikimedia.org/r/932412

Change 933999 had a related patch set uploaded (by Catrope; author: Catrope):

[design/codex@main] Accordion: Simplify styles and add comments

https://gerrit.wikimedia.org/r/933999

@bmartinezcalvo thanks, it should be fixed now

@Sswiergosz thank for all the corrections, now all looks good and the Design Review items have been checked.

One last think is about the keyboard navigation, the focus is placed in the accordion item when clicking on it, but it's not placed there when using the tab as in the rest of components (e.g. Button). Could you review this?

@bmartinezcalvo that's actually a good question. It wasn't changed by me. From what I see @egardner added tabIndex = -1 property to the accordion item. I asked about it earlier in this thread but didn't get any response so I'm not sure why it was done/if I should change it.

Change 934412 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[design/codex@main] Accordion: ARIA fixes and improvements

https://gerrit.wikimedia.org/r/934412

@Sswiergosz and @bmartinezcalvo, it looks like I made a mistake adding tabindex=-1 here – I did so based on a misunderstanding of the markup at an earlier stage of this component's development.

I've just posted a new patch that should address this problem: https://gerrit.wikimedia.org/r/934412.

@Sswiergosz and @bmartinezcalvo, it looks like I made a mistake adding tabindex=-1 here – I did so based on a misunderstanding of the markup at an earlier stage of this component's development.

I've just posted a new patch that should address this problem: https://gerrit.wikimedia.org/r/934412.

@egardner thank you! The focus works properly now. So the accordion's corrections are all done @Sswiergosz. Thank you both!

Change 933999 merged by jenkins-bot:

[design/codex@main] Accordion: Simplify styles and add comments

https://gerrit.wikimedia.org/r/933999

Change 934412 merged by jenkins-bot:

[design/codex@main] Accordion: ARIA fixes and improvements

https://gerrit.wikimedia.org/r/934412

Change 935794 had a related patch set uploaded (by Catrope; author: Catrope):

[mediawiki/core@master] Update Codex from v0.13.0 to v0.14.0

https://gerrit.wikimedia.org/r/935794

Test wiki created on Patch demo by ATomasevich (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/64dc3126c0/w

Change 935794 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v0.13.0 to v0.14.0

https://gerrit.wikimedia.org/r/935794

@CCiufo-WMF since the accordion has been successfully implemented in Codex and all corrections have been fixed, I guess we can solve this task.

@CCiufo-WMF since the accordion has been successfully implemented in Codex and all corrections have been fixed, I guess we can solve this task.

Not quite. The component is complete but it is still marked as a "WIP" component, so it is not available in the latest Codex release. I know @Catrope mentioned looking into this and can maybe weight in on next steps.

We have this documentation on transitioning the component from WIP --> Public.

Change 936087 had a related patch set uploaded (by Catrope; author: Catrope):

[design/codex@main] Accordion: Move from WIP components to public components

https://gerrit.wikimedia.org/r/936087

Change 936087 merged by jenkins-bot:

[design/codex@main] Accordion: Move from WIP components to public components

https://gerrit.wikimedia.org/r/936087

Change 939386 had a related patch set uploaded (by Jforrester; author: Eric Gardner):

[mediawiki/core@master] Update Codex from v0.14.0 to v0.15.0

https://gerrit.wikimedia.org/r/939386

Change 939386 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v0.14.0 to v0.15.0

https://gerrit.wikimedia.org/r/939386

Test wiki on Patch demo by ATomasevich (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/64dc3126c0/w/