Add React Redux to Examples #2198
Conversation
There was a problem hiding this comment.
Thank you so much @flashdesignory for the amazing work, I'm super excited to merge it alongside other Speedometer examples. I've left a few comments on certain aspects and am curious about your perspective on them. While I believe some of my suggestions could enhance our work, I recognize that they might not align with our immediate objective of updating the examples. Please view my comments as open-ended suggestions for discussion rather than definitive change requests.
| "dependencies": { | ||
| "classnames": "^2.2.5", | ||
| "prop-types": "^15.8.1", | ||
| "react": "^16.14.0", |
There was a problem hiding this comment.
Since we are updating the examples shouldn't we be using the latest react version v18?
There was a problem hiding this comment.
When we updated the Speedometer specific versions, we looked at httpArchive and NPM stats to determine, which versions are most widely used on the web today. These queries were done around March '23, so they are slightly outdated now.
| "prop-types": "^15.8.1", | ||
| "react": "^16.14.0", | ||
| "react-dom": "^16.14.0", | ||
| "react-redux": "^7.2.6", |
There was a problem hiding this comment.
Same for react-redux, the latest version is 8.1.2 as of the time of this review
There was a problem hiding this comment.
Same as comment above, 7.2.6 was the most popular version at the time we chose what version to upgrade to.
|
|
||
| import { SHOW_ALL, SHOW_COMPLETED, SHOW_ACTIVE } from "../constants/todo-filters"; | ||
|
|
||
| export default class Footer extends Component { |
There was a problem hiding this comment.
Since the more recent function components are more used and are recommended over class components by the react core team shouldn't we be using them instead of class components? I am aware that this is a big change so it might make more sense to just merge this now and update it later on.
Would love to know your opinion regarding this
There was a problem hiding this comment.
For this build, we chose a traditional class pattern to implement the todoMVC application. It's still used widely and I believe it still represents a pattern that is useful to the community.
There will be a React specific version that will use functional components and hooks to represent a more modern approach.
| @@ -0,0 +1,43 @@ | |||
| const path = require("path"); | |||
There was a problem hiding this comment.
I don't have a strong opinion regarding this but since this is a very simple app shouldn't we use something like parcel to avoid config files and make the example more simpler
There was a problem hiding this comment.
We could, this was just what we used for Speedometer. The bundler was less of a focus point for that project.
Since it works, I think this is not a show stopper, maybe a follow up task to update?
Hi @FadySamirSadek - thank you for your review 🙏 For Speedometer, we made some decisions in regards to what version to upgrade to and what type of pattern to use to implement the todoMVC application. The big callout here is that we created two React versions.
There could be an argument made to use React Toolkit vs React Redux, but that could be an additional community contribution that might be interesting to see. There are multiple ways a React + Redux todoMVC app could be created. This is just one version that we chose that worked for our purpose. |
There was a problem hiding this comment.
LGTM
If @addyosmani and @kara green light this we can go ahead and merge it
This version has been lifted from Speedometer and follows the WebKit styles guide.
For more context regarding this update, please view #2199.
@kara