Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

KingOfTac
Copy link
Collaborator

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

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

⏭ Next Steps

@KingOfTac KingOfTac self-assigned this Jan 25, 2024
@scomea
Copy link
Collaborator

scomea commented Jan 25, 2024

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

@KingOfTac
Copy link
Collaborator Author

KingOfTac commented Jan 25, 2024

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
Copy link
Collaborator Author

KingOfTac commented Jan 25, 2024

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
Copy link
Collaborator

janechu commented Jun 10, 2024

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

@janechu janechu closed this Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants