-
Notifications
You must be signed in to change notification settings - Fork 5
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
Renewal dashboard frontend #83
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.
That's so much changes - kudos to the achievement : )
I'm not a front end expert, but it does LGTM in general. One thing to improve is to add UI test in future.
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.
Not about this PR, but I found something wrong in frontend:
- The current dispatchers do no use thunk, so we can remove react-thunk, I guess.
- typo: APIRequestStatusList.failue -> failure
- no 404 page or redirect for 404
- Now you can access to
/projects/{non_existing_project_id}/data_servers
, and submit. This can be confusing although this is a minor issue.
- Now you can access to
@yuki-mt Thank you for your review.
I am not familiar with redux, so could you send us the PR for this? |
@keigohtr |
@yuki-mt
After this PR is merged, all issues will be solved! Thank you! |
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!! Great work, a lot of progress has been made :)
(I believe all tests will be passed)
What is this PR for?
Frontend renewal for the backend renewal. -> #75
This PR includes
Project
Project Access Right
redux-form
toformik
Yup
What type of PR is it?
Feature
What is the issue?
#66
#61
How should this be tested?
No frontend test available...