Skip to content

Update/collapsible nav - forwardRef#49

Merged
cchaos merged 3 commits intocchaos:update/collapsible-navfrom
thompsongl:update/collapsible-nav
Apr 26, 2021
Merged

Update/collapsible nav - forwardRef#49
cchaos merged 3 commits intocchaos:update/collapsible-navfrom
thompsongl:update/collapsible-nav

Conversation

@thompsongl
Copy link
Copy Markdown

Down to a single TS error that I don't think is resolvable. Going to sit on this for a minute and see if anything comes to mind.

Comment on lines +351 to +352
// @ts-expect-error JSX element without construct
<Element
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The remaining error. TypeScript sees div, nav, etc. as not being vaild JSX tags because (as far I can tell) as is optional, has a default value, is casted, and has a rather complex type.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I feel like this is where optional TS props fail a bit. Like as can't actually be optional because it has to render an element. It's just optional for consumers since we provide a fallback/default.

Copy link
Copy Markdown
Owner

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

@thompsongl Is this good to merge, or were you still working?

Also, are you happy enough with this implementation to establish a good pattern for other components. Maybe add an example in the wiki somehwere?

@thompsongl
Copy link
Copy Markdown
Author

This can be merged. I haven't been able to find to make this all work without at least one ts-ignore. This pattern feels the most type-safe and least likely to cause consumers extra work/trouble.

@cchaos cchaos merged commit c6c5883 into cchaos:update/collapsible-nav Apr 26, 2021
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.

2 participants