-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat(SavedTripList): Updated trip card design #1151
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.
I like the new design! At first read, this needs a11y support and some other tweaks as indicated.
width: 27px; | ||
height: 27px; | ||
border-radius: 50%; | ||
background-color: ${(props) => (props.monitored ? '#4152a4' : 'transparent')}; |
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 is nice! I think a border of the same color should be shown for all cases, so that the outline of the circle is visible for non-monitored days.
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.
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 prefer the "after" version!
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.
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.
Rats, I thought beb2dd2 fixed this but I guess not. Is it because I'm calling getBaseColor
at the top level instead of inside the MonitoredDays
component?
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.
Much better!
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.
@miles-grant-ibigroup @binh-dam-ibigroup removed the border in accordance with our tech call conversation - we can use borders on the buttons inside the form, hopefully to show that those are interactive and these aren't?
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.
@amy-corson-ibigroup Hmm I think each day should still show the circle shape somehow. Maybe in some pale grey? Contrast-wise, it should be ok, as the high-contrast circle denotes a selected day vs. a low-contrast.
…/otp-react-redux into update-trip-card
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.
Could you tweak imports, types, CSS and some of the visuals and aria-labels as indicated?
const SavedTripBody = styled.div` | ||
display: flex; | ||
justify-content: center; | ||
padding: 0 15px; |
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.
Do you have a preference for having the location icons flush or indented with respect to the the trip name heading?
width: 27px; | ||
height: 27px; | ||
border-radius: 50%; | ||
background-color: ${(props) => (props.monitored ? '#4152a4' : 'transparent')}; |
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.
Much better!
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.
Getting there! But still a bit more cleanup needed
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.
It's all nitpicks at this point but I'm being nitpicky since this css is what will define us re-doing the editor UI!
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 was an endless review process but thank you for bearing with it I think we got something wonderful in the end
const TripDetailWithIcon = styled(TextWIcon)` | ||
align-items: center; | ||
gap: 12px; | ||
// TODO: Do this in grid |
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.
Is this legal in styled-components? I'm nervous. Shouldn't it be /* comment */
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.
Styled components supports JS line comments! styled-components/stylelint-processor-styled-components#164
Thank you so much @binh-dam-ibigroup and @miles-grant-ibigroup!! |
Description:
PR Checklist:
|