Skip to content
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

Collapsible Legs in Route Overview screen #314

Merged
merged 5 commits into from
Apr 18, 2024
Merged

Collapsible Legs in Route Overview screen #314

merged 5 commits into from
Apr 18, 2024

Conversation

abhumbla
Copy link
Contributor

@abhumbla abhumbla commented Feb 8, 2024

by default collapses bike legs and only shows start/end for transit

Screenshot 2024-02-07 at 8 20 56 PM

can expand to all stops/steps

Screenshot 2024-02-07 at 8 21 52 PM Screenshot 2024-02-07 at 8 21 33 PM

@@ -26,6 +26,7 @@ export default function Icon(props) {
Icon_flipHorizontally: props.flipHorizontally,
[props.className]: !!props.className,
})}
onClick={props.onClick}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap in a button

@@ -7,6 +8,7 @@ import ItineraryBikeLeg from './ItineraryBikeLeg';
import ItineraryHeader from './ItineraryHeader';
import ItineraryTransitLeg from './ItineraryTransitLeg';
import ItineraryElevationProfile from './ItineraryElevationProfile';
import { isSignificantLeg } from './RoutesOverview';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't put utilities in component

scrollTo={scrollToLegIdx === idx}
/>
);
} else {
} else if (isSignificantLeg(leg)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't hide these by default: figure out better solution

</ItineraryDivider>,
],

{expanded ? (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try to flatten this


import './ItineraryHeader.css';

export default function ItineraryHeader({ alerts, children, icon, iconColor }) {
function alertSummary(alertBody) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to summarizeAlert

@@ -57,6 +77,36 @@ export default function ItineraryTransitLeg({ leg, onStopClick, scrollTo }) {
/>
</span>
<span>
<FormattedMessage
defaultMessage="<i>Towards {headsign}</i>"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move styling out

' transit vehicle to board.'
}
values={{ headsign: leg.trip_headsign }}
defaultMessage="<b>Board at {stop}</b>"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

description="instruction to exit (a public transit vehicle) at the named stop"
values={{ stop: stops[stops.length - 1].stop_name }}
{spacerWithMiddot}
{departure}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made start/end stops clickable

)}
<ItineraryStep IconSVGComponent={Circle} smallIcon={true}>
<FormattedMessage
defaultMessage="<b>Get off at {stop}</b>"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@@ -48,5 +54,5 @@ export default function ModeIcon({ mode, width, height, fallback = BusIcon }) {
break;
}

return <IconSvg width={width} height={height} />;
return <IconSvg width={width} height={height} onClick={onClick} />;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap this in a button

@@ -67,6 +70,10 @@ export default function Routes(props) {
dispatch(itineraryStepClicked(legClicked, stepClicked));
};

const handleIconClick = (legClicked) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to onExpandToggle

@@ -13,6 +13,7 @@ const DEFAULT_STATE = {

// State specific to an active route.
viewingDetails: false, // True if viewing detailed itinerary for the active route
viewingLeg: null, // number leg if viewing a particular leg of active route.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this state to Itinerary component

return street.totalDistance >= quartileDistance;
});

return 'via ' + streetsOverQuartile.map((s) => s.name).join(', ');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string should be localized

(It'll be cleaner if this utility function returns an array of streets and you have a UI component format the localized string)

Comment on lines 24 to 25
.ItineraryTransitLeg {
.ItineraryStep_content {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't do this, it creates maintainability issues. By convention a class like .ItineraryTransitLeg is private to the ItineraryTransitLeg component. Instead ItineraryTransitLeg should give some prop to ItineraryStep that lets it render correctly

background-color: #fff5c0;
padding: 8px;
margin: 0;
margin: 8px 0 0 0;
list-style-type: none;
border: 1px solid rgb(187, 187, 187);
border: 1px solid #f4d598;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are gradually migrating toward using Tailwind CSS and it might be an opportunity if changing the styles a lot to convert these components over.

Also, can we grab a color from Tailwind's palette for these? I've been using the Tailwind palette for minor UI stuff. Or if we want to use a custom color, it would be nice to define these new colors globally in tailwind.config.js so we can use them consistently

}) {
return (
<FormattedMessage
defaultMessage="Ride {agency} to {lastStopName}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep the textual descriptions of individual modes (bus vs train, etc) as an additional reminder in case you don't see the icon, and also for accessibility since the info is only otherwise presented visually?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

description={
'instructions header text.' +
' Says to ride the named transit line, operated by the named agency.' +
' The line is operated by a subway or metro train.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also: this statement in the description is not true)

Copy link
Contributor

@arindam1993 arindam1993 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀 🚀 🚀

@abhumbla abhumbla merged commit 2c739da into main Apr 18, 2024
2 checks passed
@abhumbla abhumbla mentioned this pull request Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants