fix warnings #324

abhumbla · 2024-04-18T03:47:42Z

got sloppy in #314

graue · 2024-04-18T16:57:22Z

This feature is looking great!

Can you fix the other unaddressed comments in #314 as well, including

avoid using <IconSvg ... onClick>: wrap in button
move viewingLeg state to <Itinerary>
localize the "via [street or streets]" string (again, see the via string for modes for an example)
don't hide short bike legs from the itinerary

Example of where hiding short bike legs is problematic: it makes it hard to figure out the 48/44 transfer.

route

abhumbla · 2024-04-18T22:22:57Z

avoid using <IconSvg ... onClick>: wrap in button

this is the only suggestion im not taking, BorderlessButton breaks styling. im using <div onClick={...}> instead

graue · 2024-04-19T01:27:03Z

How does it break styling? I'm sure there's a way to make it work. Maybe BorderlessButton needs to be improved to reset something else.

Our coding conventions say:

Don't make random elements like a div or span or icon clickable. Use a <button> so there's good keyboard, accessibility, and cursor support. Use <BorderlessButton> utility class to reset the button styling when needed.

graue · 2024-04-19T05:47:50Z

Also this comment from the original PR was not addressed in the way I intended:

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?

You added the mode name to the message description (seen by translators) but not to the actual message (in the UI). There's no reason to do this. Restoring the previous messages like "Ride the $LINE ferry ($OPERATOR)", "Ride the $LINE train ($OPERATOR)", etc., would address my comment.

abhumbla · 2024-04-19T05:50:49Z

There's no reason to do this.

i think take the BART train looks ridiculous. for people that can see the icon that's enough, the mode is there in the description for people with vision impairments

graue · 2024-04-19T06:04:41Z

Ah, I see the confusion. That's not what that description does. It's not alt text. It's a description for the translator, because they'll see the string out of context, and sometimes the same string translates differently depending on context. (For example, is "Stop" a verb or a noun?)

Personally, I don't think the icons are super clear and like having the redundancy. But if we're getting rid of the text, then the fix is to add a label for the icon. This might take a bit of refactoring because the natural place to select the label is in ModeIcon along with the icon selection, but it returns an SVG instance, not an Icon, where the label is usually attached. But it might be fine to set the aria-label on an SVG. You can right-click "Inspect Accessibility Properties" in Firefox to see if it works.

i think take the BART train looks ridiculous

For what it's worth, it would actually look like Take the Yellow-N train (BART).

graue · 2024-04-19T06:56:49Z

Hmm the itinerary no longer shows the name/number of the line either. Like I have no idea which Muni bus to board. I think this is a bigger problem

abhumbla · 2024-04-19T07:25:41Z

Hmm the itinerary no longer shows the name/number of the line either

good call, fixed

graue · 2024-04-19T07:42:43Z

src/components/ItineraryHeader.jsx


 function alertSummary(alertBody) {
-  return alertBody.slice(0, 40) + '...';
+  return alertBody.slice(0, ALERT_SUMMARY_LENGTH) + '...';


You might get more consistent results by instead using CSS to set the size (number of lines) you want to show along with overflow: hidden and text-overflow: ellipsis

graue · 2024-04-19T23:55:30Z

src/components/ItineraryBikeLeg.jsx

+              <FormattedMessage
+                defaultMessage="via {streets}"
+                description="list of major streets taken on bike leg"
+                values={{ streets: majorStreets.join(', ') }}


Not all languages use commas for punctuation so use intl.formatList for this part. There's an example in Itinerary.jsx.

graue · 2024-04-19T23:57:13Z

src/components/ItineraryHeader.jsx

+                  <span className="ItineraryHeader_alertHeader">
+                    {alertHeader}
+                  </span>
+                  <div />


what's this for?

graue · 2024-04-19T23:58:33Z

src/components/ItineraryTransitLeg.jsx

@@ -148,7 +157,7 @@ export default function ItineraryTransitLeg({
            </ItineraryStep>
          ))}
        </div>
-      ) : (
+      ) : stops.length > 2 ? (


A few lines below this there's a test for stopsBetweenStartAndEnd > 0 which seems equivalent. Is this redundant?

graue · 2024-04-19T23:59:24Z

src/features/routes.js

-      } else {
-        return { ...state, viewingLeg: action.leg };
-      }
+      return { ...state, viewingDetails: false, viewingLeg: null };


viewingLeg is no longer used

graue · 2024-04-20T00:01:01Z

src/lib/TransitModes.js

+    case MODES.TRAM_STREETCAR_LIGHT_RAIL:
+      return 'tram/streetcar/light rail';
+    case MODES.MONORAIL:
+      return 'monorail';
+    case MODES.SUBWAY_METRO:
+      return 'subway/metro';
+    case MODES.RAIL_INTERCITY_LONG_DISTANCE:
+      return 'intercity/long-distance rail';
+    case MODES.BUS:
+    case MODES.TROLLEYBUS:
+      return 'bus';
+    case MODES.FERRY:
+      return 'ferry';
+    case MODES.CABLE_TRAM:
+    case MODES.AERIAL_TRAM_SUSPENDED_CABLE_CAR:
+      return 'cable tram/cable car/aerial tram/suspended cable car';
+    case MODES.FUNICULAR:
+      return 'funicular';


These need to be localized (which means the strings need to be in a React component, for access to intl).

Also, all the slashes make it quite wordy. How about plain English? You can provide more detail to the translator in the description.

graue · 2024-04-20T00:01:34Z

src/lib/formatMajorStreets.js

 }

-function quartileStreets(streets, q) {
+async function quartileStreets(streets, q) {


Doesn't seem to do anything as there is no asynchronous stuff in the function body

graue · 2024-04-20T00:05:32Z

src/components/Itinerary.jsx

-      if (isOnlyLeg) {
-        onToggleLegExpand(idx);
+      if (isOnlyLeg && expandedLeg !== idx) {
+        setExpandedLeg(idx);


This doesn't work because React doesn't allow you to set state at render time.

If you really want to do it this way, you can use the useEffect hook to set the state in response to the legs prop changing. However, it would be cleaner and avoid an unnecessary render to incorporate this into the render logic: <ItineraryBikeLeg expanded={isOnlyLeg || expandedLeg === idx}>

also allow expanding multiple legs at one time

fix warnings

669e667

abhumbla requested a review from arindam1993 April 18, 2024 03:47

tweaks

9d7079d

abhumbla added 2 commits April 19, 2024 00:02

mode alt text

f284b44

re-add route name

8ed26c9

graue reviewed Apr 19, 2024

View reviewed changes

graue reviewed Apr 20, 2024

View reviewed changes

graue mentioned this pull request Apr 30, 2024

Fix more collapsible leg issues #328

Merged

graue added 7 commits April 29, 2024 21:54

re-internalize isSignificantLeg

b692326

localize the mode names by reusing from RouteLeg

5a11774

fix setState in render, cleanup

3c2d456

also allow expanding multiple legs at one time

remove unused viewingLeg in reducer

f8dcd91

remove useless use of async/await

aad9f7e

fully localize list of streets

1b960bf

Merge branch 'main' into fix-warnings

09d94af

graue merged commit 7207a3b into main Apr 30, 2024
2 checks passed

graue deleted the fix-warnings branch April 30, 2024 04:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix warnings #324

fix warnings #324

abhumbla commented Apr 18, 2024

graue commented Apr 18, 2024

abhumbla commented Apr 18, 2024

graue commented Apr 19, 2024 •

edited

Loading

graue commented Apr 19, 2024 •

edited

Loading

abhumbla commented Apr 19, 2024

graue commented Apr 19, 2024 •

edited

Loading

graue commented Apr 19, 2024

abhumbla commented Apr 19, 2024

graue Apr 19, 2024

graue Apr 19, 2024

graue Apr 19, 2024

graue Apr 19, 2024

graue Apr 19, 2024

graue Apr 20, 2024

graue Apr 20, 2024

graue Apr 20, 2024

fix warnings #324

fix warnings #324

Conversation

abhumbla commented Apr 18, 2024

graue commented Apr 18, 2024

abhumbla commented Apr 18, 2024

graue commented Apr 19, 2024 • edited Loading

graue commented Apr 19, 2024 • edited Loading

abhumbla commented Apr 19, 2024

graue commented Apr 19, 2024 • edited Loading

graue commented Apr 19, 2024

abhumbla commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graue commented Apr 19, 2024 •

edited

Loading

graue commented Apr 19, 2024 •

edited

Loading

graue commented Apr 19, 2024 •

edited

Loading