-
Notifications
You must be signed in to change notification settings - Fork 72
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.
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
Update v23 with React 18 + Gatsby 5 support from v22 branch #3395
base: release-23.x
Are you sure you want to change the base?
Conversation
Includes: * misc gatsby GraphQL syntax updates * fix markdown syntax issues ('<' and '{' now require escaping) * convert gatsby-config to ESM * add 'rehype-mdx-code-props' plugin to get live code blocks working * fix: "Do not define components during render" warning caused by PropsTable component that's only used in one place (duplicate props tables from "data view" page). Fixed by removing the duplicate tables. * set Content-Security-Policy header so playground works again
* used sass-migrator to auto-fix use of deprecated global functions like mix(), map-merge() * used sass-migrator to auto-fix things like @media query syntax * suppress other SASS deprecation warnings that we cannot fix at this time * update stylelint rules
Thanks for the pull request, @bradenmacdonald! This repository is currently maintained by @openedx/paragon-working-group. Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
✅ Deploy Preview for paragon-openedx-v23 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-23.x #3395 +/- ##
================================================
+ Coverage 93.88% 93.91% +0.02%
================================================
Files 232 232
Lines 3943 3943
Branches 933 901 -32
================================================
+ Hits 3702 3703 +1
+ Misses 237 236 -1
Partials 4 4 ☔ View full report in Codecov by Sentry. |
To add context to the icon changes: the browserslist config changes landed in It seems the way conflicts were resolved in there didn't keep the browserslist config changes, and I instead made CI happy by regenerating icons (8662f3f) This PR fixes that incorrect resolution from before Making that a separate PR and landing it before this merge was discussed, but that would require re-doing the merge after that PR landed. The browserslist/icon changes are isolated to a single commit on this PR and will remain in history that way. |
This PR seems to cut the "build docs" time on CI from ~4 min to ~2 min :) |
For anyone trying to review this, The "Files Changed" tab was freezing/crashing my browser tab, but the GitHub Pull Requests VSCode Extension handles this without problems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the code changes LGTM! I'm going to click around the docs site a bit and then drop a ✔️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useArrowKeyNavigationHook
, useIndexOfLastVisibleChildHook
, and useToggleHook
all have variations of
// Note: this file was renamed from 'useIndexOfLastVisibleChild.tsx' to 'useIndexOfLastVisibleChildHook.tsx' to fix
// some bugs in the Gatsby www site, where Webpack was getting the .tsx and .mdx
// files confused. Renaming this file allows us to keep the URLs of the docs site
// unchanged.
at the top of the files. This file (useIsVisibleHook
) and useWindowSizeHook
were also renamed but do not have those.
Not a blocker - just noting as something that'd be good to make consistent in release-22.x
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made an issue for this one #3396
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything seems to be working as expected on the docs site!
This PR merges the changes from #3367 into our v23 branch, as a single merge commit. There were quite a few conflicts, mostly in .scss files, but I resolved them by just replacing the SCSS changes with a new run of
sass-migrator
andstylelint --fix
, which is how the v22 SCSS changes were made.Deploy preview: https://deploy-preview-3395--paragon-openedx-v23.netlify.app/
Compare to: https://paragon-openedx-v23.netlify.app/ to avoid layout/visual differences caused by the search feature that on the "main" docs site.