-
Notifications
You must be signed in to change notification settings - Fork 377
chore(Accordion): convert examples to typescript/functional components #6716
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
chore(Accordion): convert examples to typescript/functional components #6716
Conversation
| export const AccordionDefinitionList = () => { | ||
| const [expanded, setExpanded] = React.useState('ex-toggle2'); | ||
|
|
||
| const onToggle = (id: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might turn into a broader discussion, but we might want to consider optimizing examples as they're converted to typescript.
Using this as an example, we could consider memoizing this function and including the anonymous callback used on line 19 as well.
@nicolethoen has added this as a topic to discuss in our next retro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll save most of my thoughts for the discussion tomorrow. Overall though I worry that would be jamming too many different things into one PR, unless I drop the functional conversion or extraction that I'm currently doing with these TS conversions.
nicolethoen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per today's discussion, let's make sure each functional component has an explicit return type.
|
Similar to the comment left on the TextInputGroup PR: as was discussed, and in regards to consistency that was mentioned during Wednesday's stand up, I'm thinking the file names for these separated examples should follow a consistent format such as Instead of |
thatblindgeye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of the updated example/file names, looks good!
|
Suggestion for naming convention/directory structure: What I mentioned in standup doesn't look like it's necessary, but my thought was that if these needed to be used in other places within this codebase, you could add a Since it seems these would only ever be used within the confines of /Accordion, the context of what kind of examples they are can be implied, so my vote would still be to omit the prepending of |
|
@jeffpuzzo a key part of why we thought including the component and example names would be useful is that it allows instant finding/identification of exactly what example is being worked on based on the file name alone. Additionally, it's a pattern that was already somewhat established for components, i.e. we have That being said I wouldn't be opposed to that direction, as I recognize component/filename length could become an issue. I just wanted to clarify the reasoning that we used. |
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #6715
Additional issues: