-
Notifications
You must be signed in to change notification settings - Fork 382
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
chore: bump major versions of client dependencies #529
Conversation
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.
Thanks for working on upgrading dependencies especially for transition one.
Please fix linter issues.
Also, please consider splitting changes into smaller and relevant commits. Pushing many changes in only one commit makes debugging and reviewing difficult. Many small and relevant commits are better. It helps to know the rationale behind a particular change.
@KhaledEmaraDev what's left here? |
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.
Great work 👍🏾
Thanks to @KhaledEmaraDev for starting and @Yassa-hue for wrapping up.
To me, this PR is done.
I made two comments (which are not blocking)
// `key` is a React-specific concept and is not mandatory for the | ||
// purpose of this tutorial. if you're curious, see more here: | ||
// http://facebook.github.io/react/docs/multiple-components.html#dynamic-children |
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.
Any React developer should know to include unique key
to lists to avoid issues when interacting with them. We don't need to mention it here.
// The 500 must correspond to the 0.5s in: | ||
// client/app/bundles/comments/components/CommentBox/CommentBox.module.scss:6 |
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.
I think this is an example of a good comment.
I would propose the same comment on the sass file too.
1. Bump all minor and patch versions of all packages. 2. Upgrade `autoprefixer`: Use `.browserslistrc` and delete `.postcssrc` Add it to `postcss.config.js` Install and add `postcss-preset-env` to `postcss.config.js` 3. Upgrade `immutable`: Explicitly convert all `Array`s to `List`s 4. Upgrae `marked`: Install `marked-gfm-heading-id` to support `gfm` opt 5. Upgrade `react-router`: a. Remove `exact` b. Use `Routes` instead of `Switch`. c. Import `StaticRouter` from `react-router-dom/server`. d. Use `element` instead of `component` e. Use absolute paths instead of relative in Links. f. Convert Two Components to Functional React ones in order to use `useLocation` hook. g. Use `isActive` in `className` instead of `activeClassName`. h. Use `Navigate` instead of `Redirect`. i. Reset state once redirected. 6. Upgrade `react-transition-group`: a. Use `TransitionGroup` and `CSSTransition` instead of `ReactCSSTransitionGroup`. b. Convert one component to a Functional React one to use `forwardRef` c. Add `nodeRef` as part of the state to track DOM nodes for animation 7. Install `typescript` because it's a needed peer depenency of `prettier-eslint-cli` Signed-off-by: Khaled Emara <[email protected]>
This commit solve the warning of unexpected ref object provided for a div.
- Since we create the comment nodeRef in the CommentList component We don’t need it to be created during adding a comment. - We shouldn’t change the props of a react component, It is not the best practice, So that this commit creates the nodeRef in the render function where it is used instead of the constructor function.
23b70d3
to
8c13893
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.
Good work 👍🏾
-
After rebasing, we have a new page for stimulus. There, we still get the following warning message:
-
We had a new set of browserlist. After rebase, it is gone.
-
There are some comments from my previous review. Maybe consider applying them with the next commits for wrapping this PR up.
Our only error is related to i18n, which comes from the past and can be fixed in a separate PR.
I don't see any other issues.
import { injectIntl } from 'react-intl'; | ||
import BaseComponent from 'libs/components/BaseComponent'; |
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.
It's just a linting issue.
@@ -52,9 +52,6 @@ export default class CommentList extends BaseComponent { | |||
const commentNodes = $$comments.map(($$comment, index) => { | |||
const nodeRef = React.createRef(null); | |||
return ( | |||
// `key` is a React-specific concept and is not mandatory for the | |||
// purpose of this tutorial. if you're curious, see more here: | |||
// http://facebook.github.io/react/docs/multiple-components.html#dynamic-children | |||
<CSSTransition | |||
key={$$comment.get('id') || index} |
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.
I am concerned that this is a good key. Though it works in our case I think it is better to make ids like comment_123
.
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.
LGTM 👍🏾
autoprefixer
: Use.browserslistrc
and delete.postcssrc
add it topostcss.config.js
. Install and addpostcss-preset-env
topostcss.config.js
.immutable
: Explicitly convert allArray
s toList
s.marked
: Installmarked-gfm-heading-id
to supportgfm
option.react-router
:a. Remove
exact
.b. Use
Routes
instead ofSwitch
.c. Import
StaticRouter
fromreact-router-dom/server
.d. Use
element
instead ofcomponent
.e. Use absolute paths instead of relative in Links.
f. Convert Two Components to Functional React ones in order to use
useLocation
hook.g. Use
isActive
inclassName
instead ofactiveClassName
.h. Use
Navigate
instead ofRedirect
.i. Reset state once redirected.
react-transition-group
:a. Use
TransitionGroup
andCSSTransition
instead ofReactCSSTransitionGroup
.b. Convert one component to a Functional React one to use
forwardRef
.c. Add
nodeRef
as part of the state to track DOM nodes for animation.typescript
because it's a needed peer depenency ofprettier-eslint-cli
.This change is