AlaskaAirlines / auro-accordion

Custom element that allows users to toggle the display for sections of content
https://auro.alaskaair.com/components/auro/accordion
Apache License 2.0
1 stars 1 forks source link

Only support one elements for the concept of GROUP #29

Closed blackfalcon closed 1 year ago

blackfalcon commented 2 years ago

General Support Request

Supporting two individual elements in the same scope is confusing.

Support request

We currently support a convention where if there is a single thing that is the name of the component, but then if there is a group of things, then we append -group.

I am wondering if there is a way to make this all one element? Instead of having to support two individual elements, cannot we have <auro-accordion type="group">

geoffrich commented 2 years ago

I'm against this -- I think it's a lot more readable and maintainable to have two separate elements. Right now the elements have distinct responsibilities:

  1. accordion-group ensures only one child accordion is open at one time
  2. accordion hides/shows content

These two responsibilities lead to a natural split into two elements, each in their own file. I don't have to scan through a single file, trying to figure out what pieces of code deal with type="group" and what deal with the un-typed element.

Removing the -group element means we'd have to treat the slot in the template differently depending on if type="group" is present or not. Also, type="group" could be updated at runtime by the consumer, leading to undefined behavior. With this change, there will be a lot more complexity introduced in the accordion code (more if (type === 'group')) and the implementation will become less declarative.

This API is also more verbose for consumers: <auro-accordion-group> vs <auro-accordion type="group">.

Are there any benefits I'm overlooking?

geoffrich commented 2 years ago

There's also precedent in the form of radio-group and checkbox-group -- we should stick to one style of -group API. As a consumer, I can understand the purpose of accordion-group because I'm already familiar with radio-group, or vice versa. If we changed accordion to use type="group", we would want to update those APIs as well (and I would have similar objections as what I outlined above).

blackfalcon commented 2 years ago

Yes, a precedent was set. But was that out of intent or "seems like a good idea ¯\_(ツ)_/¯" at the time?

I also agree that changing this would mean changing other group style elements. the opportunity is that all the elements you listed are also up for MAJOR releases, so the time is now if we want to embrace a new idea.

I 100% agree with your feedback that we need to be consistent. I see Spectrum struggling with this very issue. There is accordion and accordion item, then menu with menu group last there is tab and tabs.

I feel like this is an idea worth exploring versus simply persisting a concept because that was a "sure, let's do that" moment decision. And I really want to avoid whatever trap Spectrum is currently in.

geoffrich commented 2 years ago

Whatever we call it, I still like the separation-of-concerns from a maintainer standpoint that having separate elements for group and item provides. I'm not opposed to trying out the refactor to see how it feels, but we should make sure we know what problem we're trying to solve and make sure that the code remains maintainable and understandable.

And I really want to avoid whatever trap Spectrum is currently in.

What issues are you seeing with the spectrum approach? Even if the naming is inconsistent, it seems like they're still following the approach of one coordinating element (group) and child elements (items). For the reasons I outlined above, I think it would be a bad idea to collapse these two elements with distinct responsibilities into one element.