-
Notifications
You must be signed in to change notification settings - Fork 54
add stations support to nearby view #1371
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
base: dev
Are you sure you want to change the base?
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.
Only a few small cleanups needed! Mostly in getting rid of that new prop and making the new headers less bold. Can we have it match the From Here/To Here?
Also "view schedule" doesn't work
- remove rounded corners on pattern rows - hide parent schedule if there are child stops - improve styling/shadows - move view schedule button to child stop headers for stations
I think I made some big improvements in the latest commit, let me know what you think. I'm still not sure about the styling for the headers. |
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.
Great work with the CSS! I'm in agreement something is off with the headers now. Maybe same font size but higher font weight? Otherwise code looks pretty good and working well for me. Will leave it up to the second reviewer to nitpick spacings and paddings
@@ -59,11 +60,11 @@ const renderDay = (homeTimezone: string, day: number): JSX.Element => { | |||
* Represents a single pattern row for displaying arrival times in the stop | |||
* viewer. | |||
*/ | |||
// eslint-disable-next-line complexity |
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 think this can be dropped now
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.
still 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.
Are you sure
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.
Beautiful! Still a few rough edges but happy for this to go to second review. Snapshots seem messed up you might want to clear your jest cache.
@@ -59,11 +60,11 @@ const renderDay = (homeTimezone: string, day: number): JSX.Element => { | |||
* Represents a single pattern row for displaying arrival times in the stop | |||
* viewer. | |||
*/ | |||
// eslint-disable-next-line complexity |
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.
Are you sure
7a0dac4
to
a5b566c
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.
This looks great, thank you so much for the changes! How would we feel about upping the top/bottom padding on the label, increasing the margin between the first label and the "departure times" notice, and adding either a border or a background color? I think the first stop label is bleeding into the station card, adding a little separation could make it a little clearer:
Now | Border | Background |
---|---|---|
![]() |
![]() |
![]() |
(Requesting changes for the convo but nothing technically blocking here, so if we decide to keep as is I'll approve)
<FormattedMessage id="components.StopViewer.viewSchedule" /> | ||
!isParentStop ? ( | ||
<FormattedMessage id="components.StopViewer.viewSchedule" /> | ||
) : undefined | ||
} | ||
fromToSlot={fromToSlot} | ||
stopData={stopData} | ||
/> | ||
<div> | ||
<div>{timezoneWarning}</div> |
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 feel like that is too much whitespace tbh, not a fan. Especially because it's repeated on each card, it is a lot of extra space for something that isn't that important.
…nner/otp-react-redux into nearby-view-stations-basedev
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! Thanks for the changes!!
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'm happy with the border compromise! Just one small color note
…nner/otp-react-redux into nearby-view-stations-basedev
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.
What do you think about decreasing the transparency of the line?
Description:
Adds support for Stations with sub-stops in nearby view. This version is based on dev instead of the scooter merging.