PageLayout: add @deprecated notice to PageLayout.Pane position prop

[siddharthkp]

UPDATED DESCRIPTION: We are only adding @deprecated TS annotation to the position prop in this PR due to the reasons mentioned below. We also updated the docs accordingly to encourage to drop the usage of the position prop.

---

• Brings back 2 PRs:
  • [⏪ Reverted] Deprecate position prop for PageLayout.Pane, update stories and tests. #3389 by @radglob
  • [⏪ Reverted] fix(PageLayout): Remove warning for the deprecated position prop #3545 by @broccolinisoup
• Re-opened issue: https://github.com/github/primer/issues/1199
• Reverted by Revert #3389 (PageLayout.Pane: Undo deprecation for position prop) #3589
• Missing changeset! Not sure if the previous one would apply, but here it is

---

Reasons for revert, copied from #3589:

production (before)	upgrade branch (after)
Sidebar in hyperlist is on the left	Sidebar in hyperlist is on the right

• Had to revert [⏪ Reverted] Deprecate position prop for PageLayout.Pane, update stories and tests. #3389 because of a bug only caught in review-lab (not CI)
• Also had to revert [⏪ Reverted] fix(PageLayout): Remove warning for the deprecated position prop #3545 because that was built on top of 3389
• Because github does not allow reverting PRs that have changed content, had to revert the merge commit in cli

We would need to bring this change back in a way that it allows new behaviour without breaking existing behaviour.

Important: It seems like the same pull request number is also merged in next-major? Link to next-major release tracking That seems very fishy? Was this not supposed to be in main at all?

[changeset-bot]

🦋 Changeset detected

Latest commit: bb09614

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]

size-limit report 📦

Path	Size
dist/browser.esm.js	104.66 KB (0%)
dist/browser.umd.js	105.21 KB (0%)

[broccolinisoup]

@siddharthkp should we take the implementation changes back (everything on PageLayout.tsx except the deprecation notice) and can tackle how to introduce the actual deprecation in a follow up PR? Reasons for that proposal:

1. The issue seems to be opened last year, we should stop folks using the position prop at least with the deprecation notice and docs update to prevent further inaccessible usage.
2. Seems like we will eventually need to fix the usages (re-order the Pane correctly after removing the position prop) at dotcom anyway to keep the UI same. So we can buy sometime by releasing the deprecation notice and see how to introduce the changes to dotcom

Thoughts?? cc @radglob

[broccolinisoup]

Oh funny thing is that I just saw that we have a task to re-order the PageLayout.Pane if position is in used in v36 issue https://github.com/github/primer/issues/2516 we can just deprecate and handle it as a v36 follow up?

[radglob]

@siddharthkp should we take the implementation changes back (everything on PageLayout.tsx except the deprecation notice) and can tackle how to introduce the actual deprecation in a follow up PR? Reasons for that proposal:

1. The issue seems to be opened last year, we should stop folks using the position prop at least with the deprecation notice and docs update to prevent further inaccessible usage.
2. Seems like we will eventually need to fix the usages (re-order the Pane correctly after removing the position prop) at dotcom anyway to keep the UI same. So we can buy sometime by releasing the deprecation notice and see how to introduce the changes to dotcom

Thoughts?? cc @radglob

Since warnings break in dotcom, we need another way to communicate that these changes need to be made. There's a PR merged into next-major that completely removes support for position. The change that was reverted in this was intended to still respond to the position prop if present. I had tested that in storybook, but I must have missed some detail.

[broccolinisoup]

accidently approved the changes - I only wanted to leave comment. So taking it back! Sorry.

[broccolinisoup]

No worries @radglob! We removed the warnings as well because they were breaking as you said (#3545) but without the warnings, it still breaks dotcom. (Reference is the screenshot in the description and the pane moved to the right side but supposed to be on the left)

There's a PR merged into next-major that completely removes support for position.

This is great!

Since warnings break in dotcom, we need another way to communicate that these changes need to be made.

We have TS @deprecated annotation and this should be a good way to communicate the deprecation.

I feel like there is really not much else to do here. (Unless I am missing something). We can only release the @deprecated annotation in this PR and worry about the rest of the changes in the major release but seems like we already have a branch to merged to next-major to remove the support for position so maybe we are all good?

[radglob]

No worries @radglob! We removed the warnings as well because they were breaking as you said (#3545) but without the warnings, it still breaks dotcom. (Reference is the screenshot in the description and the pane moved to the right side but supposed to be on the left)

There's a PR merged into next-major that completely removes support for position.

This is great!

Since warnings break in dotcom, we need another way to communicate that these changes need to be made.

We have TS @deprecated annotation and this should be a good way to communicate the deprecation.

I feel like there is really not much else to do here. (Unless I am missing something). We can only release the @deprecated annotation in this PR and worry about the rest of the changes in the major release but seems like we already have a branch to merged to next-major to remove the support for position so maybe we are all good?

Yeah, that makes sense to me.

[radglob]

I think we should drop these changes for handling an undefined position prop. It adds extra complexity, and "end" is a reasonable default since everything should just flow as it would by source position alone.

The TS deprecation warning is enough to get the point across until next-major is merged in (which fully removes support for this).

[broccolinisoup]

Agreed! I also think we won't need the other implementation changes in this minor PR since we are only introducing a deprecation notice. Am I missing anything? I pushed a commit to take all them back and leave only the docs and deprecation notice but let me know what you think and if the other changes needed as well! @radglob

[radglob]

Yeah, that sounds right to me.

[broccolinisoup]

@radglob great! We can merge this once I clear the integration issues due to #3561

[github-actions]

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.

[lesliecdubs]

Bump, is this something we want to revive or should we close? @broccolinisoup what do you think?

[broccolinisoup]

Thanks @lesliecdubs for the ping! This was supposed to be realised in a minor version until we remove the position prop in v36.

@joshblack Do you think it still worths to release the @deprecated prop in the meantime or if there is a chance/risk that we might not be able to include this change in v36? We can act depend on how the position prop changes are going in v36.

[broccolinisoup]

Just had a chat with @joshblack and it is better to close this PR because removing the position prop couldn't make its way to v36 due to roll out complications. We can add a @deprecated notice when we have a roll out strategy for removing the position prop 🔥