Slots in SSR: revisit tradeoffs and improvements

[siddharthkp]

Context: The current slot implementation isn't ideal when it comes to server side rendering - It only renders the freeform main slot on the server and then adds other slots after mounting on client (with useEffect). Which is a non-problem for components in an 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

I need to make changes to the implementation that makes it consistent across server and client. This however will introduce a limitation that a slot needs to be a direct child of the slot container, deep nesting not allowed.

components that use slots: ActionList, FormControl and NavList by extension

so this will break the layout:

<ActionList>
+ <CustomComponent>
   <ActionList.Item>
     free text 
     <ActionList.Description>description</ActionList.Description>
   </ActionList.Item>
+ </CustomComponent>
</ActionList>

but this will not work:

<ActionList>
    <ActionList.Item>
     free text 
+   <CustomComponent> // deep nesting inside Item
       <ActionList.Description>description</ActionList.Description>
+   </CustomComponent>
   </ActionList.Item>
</ActionList>

I want to say that's a fair tradeoff to make inside an ActionList.Item.

[lesliecdubs]

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

[siddharthkp]

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

https://user-images.githubusercontent.com/1863771/180964965-df092a4e-06c3-47bd-abf7-58fe41f6adac.mp4

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

[siddharthkp]

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

[github-actions[bot]]

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.

[lesliecdubs]

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

[siddharthkp]

@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: https://github.com/primer/react/pull/2525

[lesliecdubs]

👋🏻 @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?

[colebemis]

Trending

🟢 on track

Target date

2023-04-21

This Week

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

• [x] CheckboxGroup: https://github.com/primer/react/pull/3146
• [x] RadioGroup: https://github.com/primer/react/pull/3146
• [x] FormControl https://github.com/primer/react/pull/3149

Next Week

• [ ] ActionList
• [ ] MarkdownEditor

[colebemis]

Trending

🟢 on track

Target date

2023-04-21

This Week

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

• [x] ActionList https://github.com/primer/react/pull/3173
• [x] NavList https://github.com/primer/react/pull/3173
• [x] MarkdownEditor https://github.com/primer/react/pull/3174

Next Week

• [x] Remove legacy slots implementation code

[lesliecdubs]

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