-
Notifications
You must be signed in to change notification settings - Fork 112
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
Improve event edit modal and fix bugs #113
base: main
Are you sure you want to change the base?
Conversation
c4df9db
to
5459201
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.
Thanks so much for submitting this PR! I really appreciate it. I had some smaller questions, but one big thing is that I think the "recurring event" toggle is important.
Most events are one-off events, and even users who have many recurring events will not be dealing with them very frequently in the modal because once they are set you don't have to make more of them, by definition. It doesn't make sense for the repeating event radios to be visible. In fact, it might end up causing a decent amount of accidental events if users interact with the repeating event radios instead of the datepicker, assuming that clicking Monday will make an event occur on "this monday".
Hope that makes sense! Thanks again, and let me know if you have any questions.
I agree with what you said - I looked at a couple of different event modals from different apps and they vary in their approach, but happy to go with the one you suggest. I've changed this back to include the toggle, and have the week day selector appear only when it's checked (as before). Other comments also implemented. |
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 great! One comment about making the safe button more prominent that could be good to do before merging. Im staying afk from my laptop for the weekend, but will try this out locally when I can and merge if it looks good.
flex-direction: column; | ||
} | ||
|
||
.ofc-edit-modal-event-save-button { |
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.
I wonder if the save button could be a primary interactive button here, to make it stand out from the delete button and the open note button?
This PR makes some UI changes to the event editing modal to make it cleaner and easier to understand:
Old:
New:
ToggleComponent
).It also fixes two bugs:
)
appended