microsoft / fast

The adaptive interface system for modern web experiences.
https://www.fast.design
Other
9.24k stars 590 forks source link

feat: BREAKING - rename accordion's single mode to eager and add new single mode with differing behavior #6898

Closed KingOfTac closed 3 months ago

KingOfTac commented 7 months ago

Pull Request

This is a draft to gather thoughts/feadback while I work on tests for the new single expand mode.

📖 Description

After working with Accordion in a couple different projects and researching other examples of Accordions in different UI libraries, I believe FAST's implementation is in need of an additional expand mode in order to cover more use cases.

This PR renames the existing single expand mode to eager as it eagerly expands the first non-disabled item by default. This PR also adds a new single mode that does not expand any items by default unless they already have the expanded attribute applied to them. The new single mode also allows the currently active, expanded item to be collapsed again thus reverting the Accordion back to its default state for this mode which is no items expanded.

🎫 Issues

👩‍💻 Reviewer Notes

I updated the isSingleExpandMode method to be a private getter as that seemed more appropriate for the usage of the function. Please let me know if there is something I missed here in case there are deeper reasons why this needs to be a function instead of a getter.

📑 Test Plan

Existing tests continue to pass. I did change the existing tests for single mode to be for eager mode and added some additional tests for eager mode in the few cases where I did not change the existing tests. I also added an additional story to showcase the differences between the eager and single modes.

✅ Checklist

General

Component-specific

⏭ Next Steps

scomea commented 7 months ago

Could you make it less breaky by keeping the existing "single" mode behavior and creating a new mode called "lazy" instead of "eager"?

KingOfTac commented 7 months ago

Could you make it less breaky by keeping the existing "single" mode behavior and creating a new mode called "lazy" instead of "eager"?

I could, yes. This was an attempt to try and keep the existing naming in place. I'd like to see what @chrisdholt's thoughts are as well.

I will also go double check some more examples out in the wild to see what name/behavior mappings are in use. Ideally we choose something that is as consistent as possible with other implementations so that we don't create confusion.

KingOfTac commented 7 months ago

Using the open-ui component matrix as a reference it would seem that in the few cases where an implementation has different modes for multi and single, the single mode behaves like the new single behavior this PR is introducing. The only instance of an eager and lazy mode I found was in ~Fluent UI React which calls them Exclusive and Expanded and Exclusive respectively, which it splits into two boolean attributes.~

edit: The above is from the deprecated northstar library. Fluent V9 has the default behavior set to the "eager" mode with a "collapsible" boolean that has the behavior of the "single" mode from this PR.

janechu commented 3 months ago

Closing this, due to #6951 we are only putting in fixes or critical features.