-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: support React 18; drop support for Paragon v21 in favor of v22 #441
Conversation
daec889
to
e33ca7a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #441 +/- ##
==========================================
- Coverage 78.99% 78.94% -0.06%
==========================================
Files 37 37
Lines 757 755 -2
Branches 191 191
==========================================
- Hits 598 596 -2
Misses 146 146
Partials 13 13
Continue to review full report in Codecov by Sentry.
|
fa4d6ad
to
fededc1
Compare
BREAKING CHANGE: drop support for Paragon v21 in `@edx/frontend-enterprise-catalog-search`
fededc1
to
5c178fe
Compare
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.
1 non blocking nit. Great job
@@ -1,7 +1,7 @@ | |||
import React, { useContext, useMemo } from 'react'; | |||
import React, { useContext } from 'react'; |
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.
[nit/non-blocking] Do we still need to import React
. If not is it a good opportunity to stop importing it if we don't need it.
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.
Good suggestion, but if I change it here, we should just change it everywhere. I also recall during development where I did try to remove it in a few places while working on the PR, but it complained (at least in tests) that React
couldn't be found without it imported. Will defer for separate contribution/fix.
@@ -20,7 +20,7 @@ | |||
"build": "make build", | |||
"i18n_extract": "BABEL_ENV=i18n fedx-scripts babel src --quiet > /dev/null", | |||
"lint": "fedx-scripts eslint --ext .js --ext .jsx --ext .ts --ext .tsx .", | |||
"lint:fix": "npm run test -- --fix", | |||
"lint:fix": "npm run lint -- --fix", |
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.
🥳
"@openedx/paragon": "^21.0.0", | ||
"react": "^16.12.0 || ^17.0.0", | ||
"react-dom": "^16.12.0 || ^17.0.0", | ||
"@openedx/paragon": "^22.0.0", |
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.
[inform] here is the breaking change
leftIcon: ArrowBackIos, | ||
rightIcon: ArrowForwardIos, |
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.
[inform] addressing Paragon v22 breaking change
BREAKING CHANGE: drops support for @openedx/paragon v21 in favor of v22.
feat: support React 18
Fixes
Pagination
style bug in search:Merge checklist:
frontend-app-learner-portal-enterprise
,frontend-app-admin-portal
, andfrontend-app-enterprise-public-catalog
). Will consumers safely be able to upgrade to this change without any breaking changes?BREAKING CHANGE
so the NPM package is released as such.Post merge:
chore(release): publish new versions
) that incremented versions in relevant package.json and CHANGELOG files, and created Git tags for those versions is onmaster
(Important: ensure the Git tags are for the correct commit SHA).Publish from package.json
Github Action workflow to publish these new package versions to NPM.master
branch.npm view <package_name> versions --json
).