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

2909 splitpagelayoutpane accepts an id but doesnt apply it #2922

Merged

Conversation

green6erry
Copy link
Contributor

@green6erry green6erry commented Feb 21, 2023

Describe your changes here - PageLayout/SplitPageLayout.Pane component is given an id attribute, it is applied to the component when rendered.

Closes #2909 (type the issue number after # if applicable; otherwise remove this line)

Screenshots

Please provide before/after screenshots for any visual changes
This is just showing the updated story where the id can be passed to the PageLayout/SplitPageLayout.Pane component
image

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@green6erry green6erry linked an issue Feb 21, 2023 that may be closed by this pull request
@changeset-bot
Copy link

changeset-bot bot commented Feb 21, 2023

🦋 Changeset detected

Latest commit: ea540cd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 21, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 94.65 KB (+0.03% 🔺)
dist/browser.umd.js 95.22 KB (+0.02% 🔺)

@green6erry green6erry temporarily deployed to github-pages February 21, 2023 19:20 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2922 February 21, 2023 19:21 Inactive
@green6erry green6erry temporarily deployed to github-pages February 22, 2023 15:50 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2922 February 22, 2023 15:50 Inactive
@green6erry green6erry marked this pull request as ready for review February 24, 2023 16:54
@green6erry green6erry requested review from a team and langermank February 24, 2023 16:54
@green6erry green6erry temporarily deployed to github-pages February 24, 2023 17:00 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2922 February 24, 2023 17:01 Inactive
@green6erry green6erry temporarily deployed to github-pages February 24, 2023 22:58 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2922 February 24, 2023 22:59 Inactive
@green6erry green6erry temporarily deployed to github-pages February 28, 2023 19:33 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2922 February 28, 2023 19:33 Inactive
Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

Looks great! Just left a non-blocking comment about simplifying it a bit.

@@ -736,6 +736,7 @@ const Pane = React.forwardRef<HTMLDivElement, React.PropsWithChildren<PageLayout
'--pane-max-width-diff': '959px',
},
})}
{...(id && {id: paneId})}
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about simplifying to this?

Suggested change
{...(id && {id: paneId})}
id={id}

If id is undefined, it won't pass id to the underlying component.

If id is defined, paneId and id will be the same, so we can use either.

@green6erry green6erry added this pull request to the merge queue Mar 6, 2023
Merged via the queue into main with commit 7140ac8 Mar 6, 2023
@green6erry green6erry deleted the 2909-splitpagelayoutpane-accepts-an-id-but-doesnt-apply-it branch March 6, 2023 17:03
@primer-css primer-css mentioned this pull request Mar 6, 2023
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.

SplitPageLayout.Pane accepts an id but doesn't apply it.
2 participants