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: add new "customKey" prop to ExpandableCard #2001

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

kajalex
Copy link
Contributor

@kajalex kajalex commented Oct 14, 2024

Description

Adds an optional "key" prop to the ExpandableCard react component.

When using ExpandableCard as a child component to a parent that's re-rendering on prop changes, the component appears to "flicker" due to re-rendering every time the id const changes. Internally, it's set to a new uuid() on every render. We would like to provide an optional stable value via a prop so that we don't need to regenerate the id on every render.

Screenshots

The id field utilized by Button's aria-controls and the child section
2024-10-14 14 26 54

(note: the framerate of the GIF recording doesn't catch the flickering super well, but it is visibly noticeable)

Testing in sage-lib

Rails:

  • Navigate to Expandable Card
  • See the key prop and verify that a key can be provided, which will set the id of the button element. It should be a random uuid otherwise.

React:

  • Navigate to Expandable Card
  • See the key prop and verify that a key can be provided, which will set the id of the button element. It should be a random uuid otherwise.

Testing in kajabi-products

  1. (LOW) Updates ExpandableCard to support custom key prop for assigning IDs to child elements.
    • This is a new prop and should not have any effect on existing components. Should verify consistent behavior of ExpandableCard and Accordion

Related

https://github.com/Kajabi/kajabi-products/pull/37105

@ju-Skinner ju-Skinner requested a review from a team October 16, 2024 13:22
@ju-Skinner ju-Skinner marked this pull request as ready for review October 16, 2024 13:22
Copy link
Contributor

@anechol anechol left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @kajalex.

Setting the key prop doesn't seem to work for me here as I'm only getting the uuid. If I remove the uuid and test the key alone, then the id doesn't render. However, if I change the prop's name to something like customKey then it works as expected. Do you know why this happens? I've had this issue previously with another component.

@kajalex
Copy link
Contributor Author

kajalex commented Oct 16, 2024

@anechol interesting! I was previously attempting with id and changed it to key to avoid variable name confusion. It appears that React reserves key as a prop keyword: https://legacy.reactjs.org/warnings/special-props.html
I'll go ahead and update this PR to use customKey instead.

I've seen the pattern of passing a key={"some-value"} prop into our components elsewhere in our codebase, so I'm not sure if there are issues with those existing implementations.

@kajalex kajalex force-pushed the GROW-2793/expandable-card-accept-id-prop branch from df9dc52 to 6d9f9b7 Compare October 16, 2024 16:36
@kajalex kajalex changed the title feat: add new "key" prop to ExpandableCard feat: add new "customKey" prop to ExpandableCard Oct 16, 2024
@kajalex kajalex requested a review from anechol October 16, 2024 16:40
Copy link
Contributor

@anechol anechol left a comment

Choose a reason for hiding this comment

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

This works, thanks a lot for this update, @kajalex

@anechol anechol requested a review from a team October 16, 2024 18:43
Copy link
Contributor

@QuintonJason QuintonJason left a comment

Choose a reason for hiding this comment

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

❤️

@kajalex kajalex merged commit 6dc6077 into develop Oct 16, 2024
7 checks passed
@kajalex kajalex deleted the GROW-2793/expandable-card-accept-id-prop branch October 16, 2024 21:32
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.

4 participants