Adds the accordion component based on the 'Gesso for Drupal' version #57
Conversation
There was a problem hiding this comment.
Great idea to port the accordion, @SirJohn96 ! I'm excited to see this added. You've made a good start at bringing it over. Most of my comments are variations on the same thing: a lot of the JS is still written for vanilla JS running in Drupal. For Gesso for React, we want it to be written the React way. That means making use of state, props, and components and not stepping outside of React to grab things with querySelectors.
Some of the more JS-intensive Gesso for Drupal components are still in the issue queue, so the best example I've got right now at my fingertips is the Overlay Menu: https://github.com/forumone/nextjs-project/blob/1.x/services/app/source/03-components/Menu/OverlayMenu/OverlayMenu.tsx. If you compare that with the Gesso for Drupal implementation, you can see how, for example, the HamburgerButton is now a component that gets its props from the parent component and the isOpen state variable is used to track whether or not the menu is open and set the classes and aria attributes, while the Gesso for Drupal version creates the hamburger button with another function and uses setAttribute to update the functions.
|
|
||
| useEffect(() => { | ||
| const ACCORDION_TOGGLE_CLASS = stylesAccordionItem.toggle; | ||
| const ACCORDION_SPEED = getCssVar('duration-standard'); |
There was a problem hiding this comment.
Let's make this a prop on the accordion rather than hard-coding it
| const ACCORDION_TOGGLE_CLASS = stylesAccordionItem.toggle; | ||
| const ACCORDION_SPEED = getCssVar('duration-standard'); | ||
|
|
||
| const accordion = document.getElementById(accordionId); |
There was a problem hiding this comment.
We shouldn't need to do this in React. If you need the underlying DOM element, you could use the ref, but I'm not sure we'll even necessarily need that.
| const ACCORDION_SPEED = getCssVar('duration-standard'); | ||
|
|
||
| const accordion = document.getElementById(accordionId); | ||
| const multipleAllowed = allowMultiple; |
There was a problem hiding this comment.
Since we already have the allowMultiple prop, what's the purpose of creating a new variable?
There was a problem hiding this comment.
This has been removed.
| const multipleAllowed = allowMultiple; | ||
| const toggleAllowed = allowMultiple ? true : allowToggle; | ||
|
|
||
| const openAccordion = (button: Element | HTMLElement | null) => { |
There was a problem hiding this comment.
Event handlers typically don't go in useEffect. This can be its own function that then gets called via the onClick prop on whatever triggers it.
| const toggleAllowed = allowMultiple ? true : allowToggle; | ||
|
|
||
| const openAccordion = (button: Element | HTMLElement | null) => { | ||
| if (button && button.getAttribute('aria-expanded') === 'false') { |
There was a problem hiding this comment.
This feels to me like it should be a state variable that's then used to set the aria-expanded prop on the button as well.
There was a problem hiding this comment.
This has been addressed.
| content: ''; | ||
| display: block; | ||
| height: 2px; | ||
| left: 50%; |
| height: 2px; | ||
| left: 50%; | ||
| position: absolute; | ||
| top: 50%; |
| transition-duration: var(--duration-short); | ||
| transition-property: transform; | ||
| transition-timing-function: var(--easing-ease-out); | ||
| width: 100%; |
| padding-inline: var(--spacing-2) var(--spacing-8); | ||
|
|
||
| > :last-child { | ||
| margin-bottom: 0; |
| function getCssVar(property: string): string { | ||
| // :root | ||
| const root = getComputedStyle(document.body); | ||
| return root.getPropertyValue(`--${property}`); |
|
Feel free to book any open slot on my calendar if you have questions or want to talk about anything here. |
There was a problem hiding this comment.
Really nice work, @SirJohn96 ! Just a couple small things left to tweak and then we can get this merged in.
| // Initiate accordions on page load | ||
| if (accordion) { | ||
| accordion.addEventListener('click', handleClick); | ||
| accordion.addEventListener('keydown', handleKeydown); |
There was a problem hiding this comment.
Still needs a bit of tweaking here. Instead of using addEventListener on a ref to bypass React event handling, let's do it the React way and pass the event handler as a prop.
Either <div onKeydown={handleKeydown}... or <AccordionItem onKeydown={handleKeydown}...
| <div | ||
| className={clsx( | ||
| styles.accordionItem, | ||
| isOpen ? `accordion-item_is-open` : '', |
There was a problem hiding this comment.
Is there a particular reason you're not getting this class from styles?
SirJohn96
force-pushed
the
feature-component-accordion
branch
from
effb99a to
caa1ecd
Compare
Addresses #56.