-
-
Notifications
You must be signed in to change notification settings - Fork 7
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 opening times picker with stimulus controller #1434
Conversation
Also now would be a good time for UI feedback |
8df2e86
to
1e5872e
Compare
Looking good! UI needs a little love obvi but great start. It currenlty looks like this for me - i think 'all day' checkbox last and get it all on one line but assume you're already on it. The 'all day' thing does create some funky states but I think it's no big deal. It should prob make it easier to add in the next day after selecting the current one too - maybe after entering a time it incremenets the day? |
f83fc70
to
24e6a33
Compare
27c0650
to
b2e6bd0
Compare
]); | ||
|
||
const nextDay = (day) => { | ||
const index = |
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.
tip: let nextIndex = (dayOrder.indexOf(day) + 1) % dayOrder.length;
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 okay to me
Opening a draft with the suggestion of merging this part of the work in early as it's completed.
fixes #1417
Currently doesn't fix
The last part has a lot of sprawling bits that seem to reach beyond the opening times picker and might be better approached in it's own issue/PR