Wokers list #7
Wokers list #7
Conversation
|
Here is one problem with eslint. Like there: jsx-eslint/eslint-plugin-react#2654 |
There was a problem hiding this comment.
Thanks for this. For missing props validation we would need to install another package called prop-types. More info on that in react docs here. So to add that proptype for ShowWorkers it would look something like:
ShowWorkers.propTypes = {
kick: PropTypes.bool
};
Also, currently the table on mobile screen size would be needed to scroll horizontally, should we add specific CSS for a small screens to display it somehow differently? Or this would not be possible with a table approach? If this is something that we should fix later on I am fine also.
client/src/App.js
Outdated
|
|
||
| export function App() { | ||
| const [kick, setKick] = useState(false); | ||
| const workersHandle = () => setKick(!kick); |
There was a problem hiding this comment.
Can we name this a bit more descriptive, maybe triggerFetch or triggerRefetch?
There was a problem hiding this comment.
Changed to "trigger".
client/src/service.js
Outdated
|
|
||
| export const getWorkers = async (data) => { | ||
| const response = await get("/workers", data); |
There was a problem hiding this comment.
We don't need to pass data to this function and to get method as this service is GET method and we are not sending anything to backend. They can be removed.
| .catch((err) => console.error(err)),[kick]); | ||
|
|
||
|
|
||
| if (list) { |
There was a problem hiding this comment.
Empty array is a truthy value so this if statement will always execute.
| useEffect(() => getWorkers() | ||
| .then((data) => setList(data.workers)) | ||
| .catch((err) => console.error(err)),[kick]); |
There was a problem hiding this comment.
I am thinking to have custom hook like form here https://www.digitalocean.com/community/tutorials/creating-a-custom-usefetch-react-hook. Then we would reuse it through our app and we would have loading, error and success states. On loading we could show bootstrap's Spinner. But we can add that later.
There was a problem hiding this comment.
Thanks for this @GTabala ! It's a good start...I'm not able to give detailed feedback right now, but I would like to ask that the team try to add more description to your commit messages and the PR - at least link to the Trello card. As I'm not in the day-to-day running of the team, it would make it easier for me to get the context of why this work is being done.
This is an invaluable skill to have as a developer working with professionals, and even just for yourself - if you were to revisit this code in 6 months would you remember why you made these decisions?
| @@ -1,2 +1,3 @@ | |||
| export { default as CreateWorkerForm } from "./CreateWorkerForm"; | |||
|
|
|||
There was a problem hiding this comment.
Please could you explain why you've added this commit? Thanks to version control there isn't any need to add white space - if you absolutely need to add a TODO you can add commented out lines.
There was a problem hiding this comment.
I just fixed the code according to all the comments from the reviews.
There was a problem hiding this comment.
@Harriethw I originally added that file to make it more clear and tidy when we will import them inside App.js or /pages. More info here https://www.digitalocean.com/community/tutorials/react-index-js-public-interfaces#using-indexjs
There was a problem hiding this comment.
@g1st ah I just meant that we don't need empty white space, which is all this commit seems to add unless I'm missing something?
There was a problem hiding this comment.
Yesterday we made a customer list with Hadiyah. It doesn't work on her computer. I made the same code for workers and it works. I added empty spaces for her code. :)
There was a problem hiding this comment.
Oh sorry @Harriethw yes I agree. I already proposed @GTabala to use Prettier and it can take care of those empty spaces for us!
There was a problem hiding this comment.
Prettier doesn't delete empty lines.
There was a problem hiding this comment.
Hmm, I don't know who does it then but when I save a file with loads of empty lines they get deleted for me.
|
I'm not sure that a manager will use a mobile device, but anyway, this is not time to talk about it. Even I did not create a separate component for table rows, and this should be done before the adaptation for mobile. |
|
Looks good! |
Proposal
Workers list is added. Nothing more.
Related card on Trello. We need a list of cleaners so later on admin can select specific cleaner, update, assign to a job.
Checklist
masterbranchnpm run lintand there are no errors