-
Notifications
You must be signed in to change notification settings - Fork 2
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
refactor: remove store #495
Conversation
10c2c98
to
be3fe05
Compare
🚀 Deployed on https://pr-495--dhis2-scheduler.netlify.app |
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.
Code looks good and apps seems to work as well. I only have a few suggestions, but I don't request any changes.
- I think it'd make sense to add a
src/hooks/index.js
so you don't have to import hooks from specific folders but just from../../hooks/index.js
- I've added some comments about using the
export { default as ... } from '...'
syntax, there are more files to which this comment applies, I just didn't add a comment to those files
I made this change, but it confuses the eslint import plugin. It can no longer follow the imports it seems, and we get an error about an unused module. Since this plugin is pretty useful for catching unused modules and broken imports I've reverted the change. I figured it's more important to have this properly linted than to have the benefits of the terser import style. |
6735fda
to
2436e32
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.
Looks good to me! I just did a high-level QA-type review (as I think that is what the second review was supposed to be), so just briefly skimming through the code changes and looking at the app. App looked like it worked well.
A couple of things that I noticed:
- the structure of useParameterOptions/useParameterOption hooks seems to result in unnecessary requests. Like if I select 'monitoring' as the job type, then the structure of the query also means that predictors, predictorGroups, dataIntegrity endpoints get requested even though it seems like for this job type, we just need the information from validationRuleGroups? Since the requests aren't very large and the app is more specialized, I think it's not going to cause issues, but it was something I noticed.
- it's cool that failed requests are minimally blocking, but it seems a bit hard to notice with the help text:
Also, in some cases, like with job type push analysis, the select is disabled/non-blocking, but then it seems that one cannot create a valid job. I just wonder if it would be better with something slightly more prominent, or at least a message along the lines of "try refreshing the page" in the help text? If this is the agreed design, please disregard my comment. - there's some warnings in the console
Data queries should always specify fields to return
. Not sure if you think it's worth it to update the query to get rid of those or not
Thanks for the review!
Yeah exactly, my thinking as well.
Yeah those are all good points.
Let me know what you think! |
Sounds good on those points 👍
Yeah. I was wondering the same what would be appropriate here. I guess a noticebox without a title would not be too intrusive? Maybe Joe would want to weigh in? |
Yeah that sounds good to me. I'm already doing that in several places, so it also fits well with the existing patterns. I'll add it to the follow ups. Once the queue functionality has landed we could ask Joe to take a look at these patterns. |
This reverts commit ba876c4.
# [100.4.0](v100.3.0...v100.4.0) (2023-05-15) ### Features * remove store ([#495](#495)) ([fa777b6](fa777b6))
🎉 This PR is included in version 100.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Removing the store and moving to individual data fetching hooks to prepare for the new endpoints.
Preparing for:
Follow up