Multiple plans #287
Closed
Multiple plans #287
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- updated roadmapSlice to support multiple plans
ethanwong16
added
enhancement
|
Blocker for issue #194 , @SenghoungLim to update once closed |
SenghoungLim
suggested changes
May 5, 2023
There was a problem hiding this comment.
Great job on this, it looks good. I have a few suggestions.
- I think once a user clicks on "Delete Plan", there should not be a scroll bar in the modal window. It might be confusing since we do not have more info to scroll down besides the cancel and delete buttons.
- Ties from the first suggestion, once a user adds plan name, there should not be a scroll, I think it's inconvenient to scroll down to submit the plan.
ethanwong16
requested changes
Jun 9, 2023
|
Updated from main and handled merge conflicts. Should now have staging instance, be node 18 compatible, etc. |
|
Deployed staging instance to https://staging-287.peterportal.org |
js0mmer
requested changes
Oct 27, 2023
There was a problem hiding this comment.
Great start. A few things to note:
- Additional schedules are not saved locally or to your account if logged in when clicking save
- upon reloading the page, additional schedules are lost
- only the first schedule is saved, also the name of it is reset back to "Schedule 1"
- I would prefer that you used Bootstrap for the schedule selector, delete button, and modals
- I think it makes more sense to stay consistent with the rest of the site and use Bootstrap rather than add MUI and have extra dependencies/different styling just for this feature
- Also, I'm not too sure what the emotion package is but I don't think we need to add that either
- I would look into changing the UI for managing different schedules to be more like AntAlmanac's new schedule selector, with the edit button, delete, and add all in the same place
- I think their new UI is more intuitive to use and we should copy that
- I think their new UI is more intuitive to use and we should copy that
- Lastly, consider adding a "Cancel" button to the edit name/add schedule modals
| setRoadmapPlan, addRoadmapPlan, deleteRoadmapPlan, setPlanIndex, setPlanName } = roadmapSlice.actions | ||
|
|
||
|
|
||
| // export const { setRoadmapPlan, addRoadmapPlan, deleteRoadmapPlan } = roadmapMultiplePlanSlice.actions; |
There was a problem hiding this comment.
Suggested change
| // export const { setRoadmapPlan, addRoadmapPlan, deleteRoadmapPlan } = roadmapMultiplePlanSlice.actions; |
Remove if not needed.
| label={name} | ||
| onChange={(e) => { | ||
| let newIndex = +(e.target.value); | ||
| // console.log("selected index", newIndex); |
There was a problem hiding this comment.
Suggested change
| // console.log("selected index", newIndex); |
I agree with this; maybe we could put it where it currently says "Peter's Roadmap"? |
js0mmer
force-pushed
the
multiple-plans
branch
from
5ac93af to
dcd3f6e
Compare
|
Progressed picked up in #472 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Screenshots
Steps to verify/test this change:
Final Checks:
(optional)