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

[Batch] Improve slots implementation in PRC to enable React SSR compatibility & performance gain #1690

Closed
10 tasks done
siddharthkp opened this issue Dec 3, 2021 · 10 comments · Fixed by #3205
Closed
10 tasks done

[Batch] Improve slots implementation in PRC to enable React SSR compatibility & performance gain #1690

siddharthkp opened this issue Dec 3, 2021 · 10 comments · Fixed by #3205
Assignees
@colebemis
Labels
epic react size: rock
Milestone
FY23 - Q4

Comments

@siddharthkp
Copy link
Member

siddharthkp commented Dec 3, 2021 •
edited by colebemis
Loading

Background

The current slot implementation isn't compatible with server-side rendering (SSR). It only renders the freeform main slot on the server and then adds other slots after mounting on the client (with useEffect) which is not a problem in client-only components like Overlay but can be a bad loading experience for other components with layout shift.

The tradeoffs considered at the time of building this component are mentioned in this comment: https://github.com/github/primer/issues/1224#issuecomment-1222450075

Task

Update the slots implementation to render consistently across the server and client. This will introduce a limitation that a slot needs to be a direct child of the slot container, deep nesting is not allowed.

Components to migrate

Related work

Clean up
@siddharthkp siddharthkp self-assigned this Dec 3, 2021
@siddharthkp siddharthkp mentioned this issue Dec 6, 2021
Merged
6 tasks
@gaknoll gaknoll added the bug Something isn't working label Jan 20, 2022
@gaknoll gaknoll added this to the FY22 - Q3 milestone Jan 20, 2022
@siddharthkp siddharthkp mentioned this issue Feb 8, 2022
Merged
6 tasks
@lesliecdubs
Copy link
Member

We probably want to pick this one up when we work on Navigation List https://github.com/github/primer/issues/637.

This was referenced Jun 3, 2022
Closed
Closed
@siddharthkp siddharthkp changed the title Slots in SSR: document tradeoffs and improvements Slots in SSR: revisit tradeoffs and improvements Jun 3, 2022
@siddharthkp siddharthkp removed this from the FY22 - Q3 milestone Jul 25, 2022
@siddharthkp
Copy link
Member Author

siddharthkp commented Jul 26, 2022 •
edited
Loading

Another use case for this (without SSR) - https://github.com/orgs/github/issues/issues (behind feature flag)

Screen.Recording.2022-07-25.at.10.19.03.online-video-cutter.com.1.mp4

As a stop gap, we moved the contents of leading visual inside text

@siddharthkp
Copy link
Member Author

Another example where this falls short: https://github.com/github/primer/issues/1224

@siddharthkp siddharthkp mentioned this issue Jan 25, 2023
Closed
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Feb 18, 2023
@lesliecdubs
Copy link
Member

Hey @siddharthkp, would it be helpful to keep this one open for reference or should we go ahead and close? I have a draft in the Primer Roadmap backlog to revisit slots so it's definitely still on the radar. https://github.com/orgs/github/projects/2759/views/32?filterQuery=-no%3Afocus-area+-status%3A%22Inbox+%F0%9F%93%A5%22+-label%3Ainitiative+slots

@github-actions github-actions bot removed the Stale label Feb 22, 2023
@siddharthkp
Copy link
Member Author

@lesliecdubs Late reply, but I think we should keep this open :)

Leaving here for future reference, we have a draft of the implementation in this closed PR: #2525

@colebemis colebemis mentioned this issue Mar 15, 2023
Merged
6 tasks
@colebemis colebemis self-assigned this Mar 17, 2023
@colebemis colebemis mentioned this issue Mar 21, 2023
Merged
@colebemis colebemis mentioned this issue Mar 30, 2023
Merged
@lesliecdubs lesliecdubs changed the title Slots in SSR: revisit tradeoffs and improvements [Batch] Improve slots implementation in PRC to enable React SSR compatibility & performance gain Mar 30, 2023
@lesliecdubs lesliecdubs added epic and removed bug Something isn't working labels Mar 30, 2023
@lesliecdubs lesliecdubs added this to the FY23 - Q4 milestone Mar 30, 2023
@lesliecdubs
Copy link
Member

👋🏻 @colebemis starting this week, could you please add a /status update on this batch each week (link for GitHub staff only) assuming this is still something you have in progress?

This was referenced Apr 10, 2023
Merged
Merged
@colebemis
Copy link
Contributor

Trending

🟢 on track

Target date

2023-04-21

This Week

Migrated CheckboxGroup, RadioGroup, and FormControl to use the SSR-compatible slots implementation:

Next Week

  • ActionList
  • MarkdownEditor

This was referenced Apr 14, 2023
Merged
Merged
@colebemis
Copy link
Contributor

colebemis commented Apr 21, 2023 •
edited
Loading

Trending

🟢 on track

Target date

2023-04-21

This Week

Migrated ActionList, NavList, and MarkdownEditor to use the SSR-compatible slots implementation:

Next Week

  • Remove legacy slots implementation code

@colebemis colebemis mentioned this issue Apr 21, 2023
Merged
@lesliecdubs
Copy link
Member

Hooray! 🎉 Thank you for the quick work on this one @colebemis!

Young man in a black t-shirt claps emphatically and mouths the word "YES" with an intense face

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@colebemis colebemis

Labels
epic react size: rock
Projects
None yet
Milestone
FY23 - Q4
Development

Successfully merging a pull request may close this issue.

Deprecate legacy slots implementation primer/react
4 participants
@siddharthkp @lesliecdubs @colebemis @gaknoll