-
Notifications
You must be signed in to change notification settings - Fork 35
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
Update Date/Time Selector #793
base: master
Are you sure you want to change the base?
Update Date/Time Selector #793
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.
We definitely need this to be a breaking change! Please add a breaking change commit. Stories look great! Haven't looked at the code yet
BREAKING CHANGE: Replaces the old d/t selector in favor of new design
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.
First round of fixes! There's some layout problems in some contexts and some code cleanup possible too
> | ||
<S.DateTimeSelector.DepartureRow> | ||
<Dropdown alignMenuLeft id="date-time-depart-arrive" text={departureOptions.find(opt => opt.isSelected).text} buttonStyle={{ backgroundColor: S.baseColor() || blue[900], borderRadius: "3px 0px 0px 3px", color: "white", height: "45px", border: "0px", padding:"5px 7px" }}> |
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.
Can we make the default gray instead of blue?
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.
Also some of these values are pretty fixed and are causing layout problems in some implementations. Any way we can make this more reactive to its environment?
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.
We approved the designs with the blue, also I think generally we've been using blue[900] to represent the styled base color, so using grey seems a little inconsistent.
Can you elaborate on which values are causing issues? I'm seeing the height but a lot of form elements have fixed heights. Took out the padding though because it was redundant!!
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.
Try with any non-blue config! The blue looks very out of place. As for getting render issues try loading a config that was migrated from the batch ui and then resize the window
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.
Also, the metro mode selector's default color is gray. Can we match that?
departureOptions.forEach(opt => { | ||
opt.isSelected = departArrive === opt.type; | ||
}); |
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.
This works but it's pretty inefficient... Anyway we can replace/generate isSelected
when we're iterating through the array anyway?
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.
Added! We do have to add the check in a few places, let me know what you think!
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.
Mostly looking good, just the blue is still bothering me. Still have to do some more integration tests as well
> | ||
<S.DateTimeSelector.DepartureRow> | ||
<Dropdown alignMenuLeft id="date-time-depart-arrive" text={departureOptions.find(opt => opt.type === departArrive).text} buttonStyle={{ backgroundColor: S.baseColor() || blue[900], borderRadius: "3px 0px 0px 3px", color: "white", height: "45px", border: "0px" }}> |
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.
We can probably set extract S.baseColor() || blue/gray
?
Updates the D/T selector styling so that date time inputs are always visible, and the departure option (leave now, depart at, arrive by) is a dropdown.
If a user updates the date or time input when the option "leave now" is selected, the dropdown will change to "depart at".