Skip to content

Add internal ConditionalWrapper component - #4095

Merged
langermank merged 3 commits into
mainfrom
internal-comp-conditional-wrapper
Mar 22, 2024
Merged

Add internal ConditionalWrapper component#4095
langermank merged 3 commits into
mainfrom
internal-comp-conditional-wrapper

Conversation

@langermank

@langermanklangermank commented Dec 20, 2023

Copy link
Copy Markdown
Contributor

I have a use case for this in Button, and we have this same component in PVC.

I renamed it to match up with PVC, but no changes beyond that. I've also tested it already in #3582.

@langermank
langermank requested review from a team and mperrottiDecember 20, 2023 16:59
@changeset-bot

changeset-botBot commented Dec 20, 2023•
edited
Loading

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 682998b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@langermanklangermank added the skip changeset This change does not need a changelog label Dec 20, 2023
@github-actions

github-actionsBot commented Dec 20, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
packages/react/dist/browser.esm.js88.57 KB (-0.02% 🔽)
packages/react/dist/browser.umd.js88.85 KB (-0.16% 🔽)

@github-actions
github-actionsBottemporarily deployed to storybook-preview-4095 December 20, 2023 17:03 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4095 December 20, 2023 18:36 Inactive
@joshblack

joshblack commented Dec 20, 2023

Copy link
Copy Markdown
Member

I think one downside of this technique that wouldn't exist if using ternaries or refactoring into smaller components is that things are evaluated even if the condition is false-y.

So when we have code like:

<ConditionalWrapperif={conditional}sx={{/* ... */}}><ChildComponentsx={{/* ... */}}><SomeOtherChild/></ChildComponent></ConditionalWrapper>

Things like the sx props are being generated along with anything that are children within the component. This can make some types of code awkward if you're relying on something from the if block to be defined for a child component and TS won't properly narrow like it would when using ternaries or refactoring into sub-components.

<ConditionalWrapperif={hasTheThing}>{/* this will be evaluated even if `hasTheThing` is false-y */}{/* it also won't be narrowed so it will require a guard */}<SomeComponent>{hasTheThing.title}</Subcomponent/></ConditionalWrapper>

I think with this in mind I'd love to encourage alternatives that we could use instead that wouldn't have these limitations. Ternaries are definitely something that come to mind but they are so inconvenient/hard to understand with a lot of nesting. In those cases, it may be worth us trying to refactor things into sub-components that are local to the file to help avoid the challenges above while still being able to reason about why things are rendering.

@pksjcepksjce mentioned this pull request Dec 21, 2023
Merged
7 tasks
@langermank
langermank marked this pull request as draft December 21, 2023 15:55
@langermank

langermank commented Dec 21, 2023

Copy link
Copy Markdown
ContributorAuthor

Putting this on pause as we look into alternative solutions ✨

@mperrotti
mperrotti self-requested a review December 21, 2023 17:19
@mperrotti

mperrotti commented Dec 21, 2023

Copy link
Copy Markdown
Contributor

Dismissing my review until we agree on a path forward from this discussion

@mperrotti

mperrotti commented Dec 21, 2023

Copy link
Copy Markdown
Contributor

It's worth mentioning that we use a similar component in ActionList.Item: https://github.com/primer/react/blob/main/src/ActionList/Item.tsx#L353

@github-actions

github-actionsBot commented Feb 19, 2024

Copy link
Copy Markdown
Contributor

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

@github-actions
github-actionsBot deleted the internal-comp-conditional-wrapper branch February 26, 2024 19:04
@mperrotti
mperrotti restored the internal-comp-conditional-wrapper branch March 12, 2024 15:25
@mperrottimperrotti reopened this Mar 12, 2024
@mperrotti

mperrotti commented Mar 12, 2024

Copy link
Copy Markdown
Contributor

Reopening since we have a path forward for live region support: #4313

@mperrotti
mperrotti marked this pull request as ready for review March 22, 2024 21:23
@mperrotti
mperrotti requested a review from a team as a code ownerMarch 22, 2024 21:23
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4095 March 22, 2024 21:24 Inactive
@langermank
langermank added this pull request to the merge queueMar 22, 2024
Merged via the queue into main with commit b812dd7Mar 22, 2024
@langermank
langermank deleted the internal-comp-conditional-wrapper branch March 22, 2024 21:59
lukasoppermann pushed a commit that referenced this pull request Apr 16, 2024
@langermank@mperrotti@lukasoppermann
Add internalConditionalWrappercomponent (#4095)
1f4d2ed
* move to internal comp
* lint
---------
Co-authored-by: Mike Perrotti <mperrotti@github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@mperrottimperrottimperrotti approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip changesetThis change does not need a changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@langermank@joshblack@mperrotti