Repository navigation
chore: more ssr! - #9260
chore: more ssr!#9260avivkeller wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9260 +/- ##
==========================================
- Coverage 86.14% 84.81% -1.33%
==========================================
Files 87 108 +21
Lines 6164 7152 +988
Branches 369 428 +59
==========================================
+ Hits 5310 6066 +756
- Misses 850 1078 +228
- Partials 4 8 +4 ☔ View full report in Codecov by Harness. |
📦 Build Size ComparisonSummary
Changes🔄 Modified Routes (4)
|
| import styles from './index.module.css'; | ||
|
|
||
| type DownloadButtonProps = { release: NodeRelease }; | ||
| type DownloadButtonProps = { release: NodeRelease } & Pick< |
There was a problem hiding this comment.
Why is this better? Those functions are already SSR.
| const release = use(ReleaseContext); | ||
| const t = useTranslations(); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Why are you removing this? This was intentional...
|
|
||
| const PlatformDropdown: FC = () => { | ||
| const { architecture, bitness } = useClientContext(); | ||
| const { architecture, bitness } = use(DetectedOSContext); |
There was a problem hiding this comment.
Make a Hook for the Provider please 🙇
| // If there's only a single navigation key, use its sub-items as our navigation | ||
| const groups = ( | ||
| navKeys.length === 1 ? sideNavigation[0].items : sideNavigation | ||
| ).map(toGroup); |
There was a problem hiding this comment.
const groupToUse = navKeys.length === 1 ? sideNavigation[0].items : sideNavigation
const groups = groupToUse.map(toGroup);
ovflowd
left a comment
There was a problem hiding this comment.
As usual, please, add PR description, explain the changes, how they tie together, add screenshots, if there's any side-effect and confirm that you have tested every single one of these pages and confirmed all sort of behaviors in all sort of scenarios that the previous code held are still working.
It's hard to validate when a PR is touching so many different things seeminlgy not tied together (+ changing styles)
| setOpen(localStorage.getItem(STORAGE_KEY) !== banner.text); | ||
| } | ||
| // eslint-disable-next-line @eslint-react/set-state-in-effect | ||
| setOpen(localStorage.getItem(STORAGE_KEY) !== banner.text); |
There was a problem hiding this comment.
nit: we absoluely avoid breaking the glass within a React component. In this case, Storage APIs, we should use a Hook for reading/writing from Storage.
| return label ? [...breadcrumbs, { label, href: link }] : breadcrumbs; | ||
| } | ||
| // Reduce the path to a breadcrumbs array by finding each segment in the current navigation layer, | ||
| // descending into the found node's items (next layer) for the next segment. |
There was a problem hiding this comment.
nit: move this fn to an utility file, keeps the component just with the component. Allows you to unit test getBreadcumbs
| @@ -51,12 +43,7 @@ const MetaBar: FC = () => { | |||
| ...(usernames.length && { | |||
| [t( | |||
There was a problem hiding this comment.
separate this t() call to a constant.
| const actions = useMemo(() => getActions(dispatch), [dispatch]); | ||
|
|
||
| // The detected OS and platform seed the selection once they are known | ||
| useEffect(() => { |
There was a problem hiding this comment.
I think there were reasons why we left each one of these Hooks contained to the Components that own such functionality. i.e. the OS component has Hooks for OS changes.
I'm unsure it is a good design pattern for a Provider to hold App Logic specific to certain components that use said data, and where said data changes only matter for them...
There was a problem hiding this comment.
Why are we removing the Matter Provider from here? 🤔
|
|
||
| import styles from './index.module.css'; | ||
|
|
||
| const COMPACT_MEDIA_QUERY = '(max-width: 1280px)'; |
There was a problem hiding this comment.
nit: constants to live in constants files
| </BreadcrumbItem> | ||
| )} | ||
| {isOverflow && <BreadcrumbTruncatedItem />} | ||
| {hiddenCount > 0 ? ( |
| [styles.currentDropDown]: code === currentLanguage, | ||
| })} | ||
| // A plain anchor has no locale to switch to | ||
| {...(Component !== 'a' && { locale: code })} |
There was a problem hiding this comment.
But do we really need... this? What happens if we simply pass locale to a? It becomes a HTML prop right?
(1) Look at https://webpack-doc-kit.vercel.app/docs/api/v5.x and you'll see the issue (speed!)
(2) This change can cascade (with some other similar changes) to make the site a ton smaller using more SSR!