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.

Sign up for GitHub

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

Jump to bottom

[⏪ Reverted] fix(PageLayout): Remove warning for the deprecated position prop #3545

Merged
merged 11 commits into from
Jul 27, 2023
Merged

[⏪ Reverted] fix(PageLayout): Remove warning for the deprecated position prop #3545

merged 11 commits into from
Jul 27, 2023

Conversation

broccolinisoup
Copy link
Member

@broccolinisoup broccolinisoup commented Jul 21, 2023 •
edited
Loading

deprecating PageLayout.Pane PR introduces a warning then the prop is used and although this is a good practise, it is not dotcom-friendly. Because it fails dotcom tests since warnings are not expected.

For the time being, we are settling for removing the warnings and only going with the TS deprecated notice. This PR removes the warning and adds the TS deprecated notice.

For more context, please see the slack thread here (GitHub stuff only)

Screenshots

No visual change

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Changes are SSR compatible
  • 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.

@broccolinisoup broccolinisoup added the skip changeset This change does not need a changelog label Jul 21, 2023
@changeset-bot
Copy link

changeset-bot bot commented Jul 21, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: d20aa6e

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 changesets to release 1 package
Name Type
@primer/react Patch

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

@github-actions
Copy link
Contributor

github-actions bot commented Jul 21, 2023 •
edited
Loading

size-limit report 📦

Path Size
dist/browser.esm.js 102.4 KB (-0.09% 🔽)
dist/browser.umd.js 102.99 KB (-0.09% 🔽)

@broccolinisoup broccolinisoup temporarily deployed to github-pages July 21, 2023 00:36 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3545 July 21, 2023 00:36 Inactive
@broccolinisoup broccolinisoup temporarily deployed to github-pages July 21, 2023 01:20 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3545 July 21, 2023 01:21 Inactive
@broccolinisoup broccolinisoup temporarily deployed to github-pages July 21, 2023 01:39 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3545 July 21, 2023 01:40 Inactive
@broccolinisoup broccolinisoup temporarily deployed to github-pages July 21, 2023 02:26 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3545 July 21, 2023 02:26 Inactive
@broccolinisoup broccolinisoup force-pushed the bs/pagelayout-position-deprecation branch from 30e394a to 843e304 Compare July 21, 2023 03:53
@broccolinisoup broccolinisoup temporarily deployed to github-pages July 21, 2023 04:02 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3545 July 21, 2023 04:02 Inactive
@broccolinisoup broccolinisoup temporarily deployed to github-pages July 21, 2023 04:10 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3545 July 21, 2023 04:10 Inactive
@broccolinisoup broccolinisoup force-pushed the bs/pagelayout-position-deprecation branch from 3c5224d to fdcd281 Compare July 21, 2023 04:29
@broccolinisoup broccolinisoup temporarily deployed to github-pages July 21, 2023 04:35 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3545 July 21, 2023 04:35 Inactive
@broccolinisoup broccolinisoup changed the title Add deprecated notice and run console warn only in DEV fix(integration): do not run console warn in test PageLayout & revert ToggleSwitch & fix markup structure for UnderlineNav Jul 21, 2023
@broccolinisoup broccolinisoup changed the title fix(integration): do not run console warn in test PageLayout & revert ToggleSwitch & fix markup structure for UnderlineNav fix(integration): do not run console warn in test PageLayout Jul 25, 2023
@github-actions github-actions bot temporarily deployed to storybook-preview-3545 July 25, 2023 05:39 Inactive
@broccolinisoup broccolinisoup temporarily deployed to github-pages July 25, 2023 05:41 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3545 July 25, 2023 05:41 Inactive
joshblack
joshblack approved these changes Jul 26, 2023
View reviewed changes
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

+1 for TS @deprecated, I'd vote for just removing the warning in console personally and only use @deprecated till we find a better pattern for warnings here.

Here's the snippets we've been using for the changelog if it helps for messaging! (Also let me know if they're incorrect 😅)

  /**
   * @deprecated Use source order instead of relying on the `position` prop
   *
   * Before:
   * ```
   * <PageLayout>
   *   <PageLayout.Content />
   *   <PageLayout.Pane position="start" />
   * </PageLayout>
   * ```
   *
   * After:
   * ```
   * <PageLayout>
   *   <PageLayout.Pane />
   *   <PageLayout.Content />
   * </PageLayout>
   * ```
   */

@joshblack joshblack mentioned this pull request Jul 26, 2023
Closed
4 tasks
@broccolinisoup broccolinisoup marked this pull request as ready for review July 26, 2023 21:20
@broccolinisoup broccolinisoup requested review from a team and siddharthkp July 26, 2023 21:20
@broccolinisoup broccolinisoup changed the title fix(integration): do not run console warn in test PageLayout fix(PageLayout): Remove warning for the deprecated position prop Jul 27, 2023
@broccolinisoup broccolinisoup temporarily deployed to github-pages July 27, 2023 00:27 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3545 July 27, 2023 00:28 Inactive
@broccolinisoup broccolinisoup added this pull request to the merge queue Jul 27, 2023
Merged via the queue into main with commit 430a203 Jul 27, 2023
@broccolinisoup broccolinisoup deleted the bs/pagelayout-position-deprecation branch July 27, 2023 01:08
siddharthkp added a commit that referenced this pull request Aug 4, 2023
@siddharthkp
Revert "fix(PageLayout): Remove warning for the deprecated `positio…
5790b5d 
…n` prop (#3545)"

This reverts commit 430a203.
This was referenced Aug 4, 2023
Merged
Closed
github-merge-queue bot pushed a commit that referenced this pull request Aug 4, 2023
@siddharthkp
Revert #3389 (PageLayout.Pane: Undo deprecation for position prop) (#…
d598a53 
…3589)

* Revert 57c4bdf

* Create nervous-dolls-dream.md

* Update nervous-dolls-dream.md

* Revert "fix(PageLayout): Remove `warning` for the deprecated `position` prop (#3545)"

This reverts commit 430a203.

* linter caught a bad merge!

* Remove reverted changeset

* copy old test

* Update nervous-dolls-dream.md
@siddharthkp siddharthkp changed the title fix(PageLayout): Remove warning for the deprecated position prop [⚠️ Reverted] fix(PageLayout): Remove warning for the deprecated position prop Aug 7, 2023
@siddharthkp siddharthkp changed the title [⚠️ Reverted] fix(PageLayout): Remove warning for the deprecated position prop [⏪ Reverted] fix(PageLayout): Remove warning for the deprecated position prop Aug 7, 2023
@siddharthkp siddharthkp mentioned this pull request Aug 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshblack joshblack joshblack approved these changes

@siddharthkp siddharthkp Awaiting requested review from siddharthkp

Assignees
No one assigned
Labels
skip changeset This change does not need a changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@broccolinisoup @siddharthkp @joshblack