Skip to content

Conversation

@Francois-Esquire
Copy link
Contributor

@Francois-Esquire Francois-Esquire commented Jul 31, 2023

  • BREAKING CHANGE?

Description

Adds a new sections prop to explicitly define each section. Moving forward, sections will be needed for the useAccordion container to work properly. There have been additional changes to the distribution exports.

Checklist

  • 🌐 demo is up-to-date (yarn start)
  • 💂‍♂️ includes new unit tests
  • ♿ tested for WCAG 2.1 AA compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

@Francois-Esquire Francois-Esquire requested a review from a team as a code owner July 31, 2023 18:56
@coveralls
Copy link

coveralls commented Jul 31, 2023

Coverage Status

coverage: 95.623% (-0.04%) from 95.665% when pulling 501fe2c on mike/refactor/accordion into 8a30d4b on main.

Copy link
Contributor

@geotrev geotrev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to chat in more depth if anything is unclear. Thanks


const isControlled = expandedSections !== null && expandedSections !== undefined;
const [expandedState, setExpandedState] = useState<Value[]>(
defaultExpandedSections || expandedSections || sections.slice(0, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this falling back to an expansion state on the first section? Feels like we should be accepting undefined as a valid state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain more on this? expandedState does have dependents (like disabledState, etc) which does need it to be an Array in certain conditions.

Copy link
Contributor

@geotrev geotrev Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly related to JZ's comment, but I would expect expandedSections to come before defaultExpandedSections - controlled value takes priority, and the default as the fallback, if given.

Getting back to JZ's comment, I'm curious on the thinking there as well. Not opposed to it, but keeping the type consistent does help with the internal logic type reliability from the get go. I could also see defaultExpandedSections having a default arg value of [] to make it simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 31-36 use the internalExpandedState which resolves to the expandedSections before defaultExpandedSections 😄:

// lines 31-36
  const isControlled = expandedSections !== null && expandedSections !== undefined;
  const [expandedState, setExpandedState] = useState<Value[]>(
    defaultExpandedSections || sections.slice(0, 1)
  );
  const [disabledState, setDisabledState] = useState(collapsible ? [] : expandedState);
  const internalExpandedState = getControlledValue(expandedSections, expandedState)!;

On the second point, it will change the logic indeed.

Copy link
Contributor

@geotrev geotrev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the closer alignment with with other containers a la sections. Other than JZ's remaining comments, it's looking much better!

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, @Francois-Esquire

@Francois-Esquire Francois-Esquire merged commit e5cc2e1 into main Aug 3, 2023
@Francois-Esquire Francois-Esquire deleted the mike/refactor/accordion branch August 3, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants