Added missing GTFS Extended Route Types #656

robertgregg3 · 2021-04-08T16:51:14Z

Added the route types to the gtfs.yml file
~~and then added logic to display the route_type field as an optgroup~~
Create a new selector for GTFS route types, with extended route types organized as a tree hierarchy (bus, metro, etc) that is not expanded by default to reduce screen clutter while searching for extended route types.

Added the route types to the GTFS.yml file and then added logic to display the route_type field in an optgroup #645

landonreed · 2021-04-08T17:29:07Z

lib/editor/components/EditorInput.js

@@ -300,6 +300,8 @@ export default class EditorInput extends Component<Props> {
          </span>
        )
      case 'DROPDOWN':
+        const standard = 'Standard'
+        const extended = 'Extended'
        return (


To fix the flow type issues and simplify things, I would add something like the following:

if (!field.options) { console.warn(`Field `field.name` does not have options configured, but is marked as inputType=DROPDOWN`) return null }

Then, you should be able to remove the field.options && ... checks on lines 315 and 330.

Where do you see the flow issues? When I load the project (before typing any code, I get these kind of squigglies from the beginning of the render method:

When I run flow status I get an error:

The flow errors are showing up in the GitHub builds: https://github.com/ibi-group/datatools-ui/pull/656/checks?check_run_id=2298915284

Locally, you should run yarn flow.

landonreed · 2021-04-08T17:33:03Z

gtfs.yml

@@ -290,6 +290,156 @@
      text: Gondola
    - value: 7
      text: Funicular


Could you also add options 11 and 12 (they were recently added to the standard set)? See the spec for routes.txt: https://developers.google.com/transit/gtfs/reference#routestxt for more info

lib/editor/components/EditorInput.js

landonreed

A few changes, but this is looking good!

binh-dam-ibigroup · 2021-04-09T14:19:04Z

gtfs.yml

+    - value: 100
+      text: Railway Service


Why is this value duplicated?

Good spot. And there was a word missing. Going to check the rest now.

binh-dam-ibigroup · 2021-04-09T14:41:23Z

lib/editor/components/EditorInput.js

-              {field.options && field.options.map(o => (<option value={o.value} key={o.value}>{o.text || o.value}</option>))}
+              {field.name === 'route_type' &&
+                <optgroup label={standard}>
+                  {field.options.slice(0, 7).map(o => (


Because o shows up so many times, I would write .map(({ text, value }) => { ..., and maybe also extract the <optgroup> rendering to its own method because of repetition.

Added a group option to the gtfs route types options. Added method to render the route type options. #656

robertgregg3 · 2021-04-20T12:49:41Z

I have added:

a group option in the gtfs file
added a _renderRouteTypeOptions method to dynamically render the opt groups depending on the group value (Standard or Extended)

landonreed

@robertgregg3, what prompted the addition of the group field in gtfs.yml? I'm not sure it's needed, given that we effectively know that types 0-12 are standard and those after are extended. I liked the way you had handled it previously.

Also, Trolleybus (11) and Monorail (12) should probably be in standard and moved up in the list. These are currently shown as extended (800 and 405), but I think that's probably just because the extended list has not been updated. We could keep them both, but that might be confusing.

binh-dam-ibigroup

Almost there, see how the simplification suggestion would work with @landonreed's question regarding the groups of route types.

binh-dam-ibigroup · 2021-04-20T19:04:38Z

lib/editor/components/EditorInput.js

+    for (const option in fieldOptions) {
+      if (fieldOptions[option].group === 'Standard') {
+        standardTypes.push(fieldOptions[option])
+      } else {
+        extendedTypes.push(fieldOptions[option])
+      }
+    }


This could be simplified:

const standardTypes = Object.values(fieldOptions).filter(fieldOption => fieldOption.group === 'Standard') // same for extendedTypes

Even better, in the return statement, try this:

return routeTypes .map(type => Object.values(fieldOptions).filter(/*see above*/)) .map(routeTypes => { <optgroup> {routeTypes.map(routeType => /*what you already have for each std/extended types*/)} </optgroup> })

Nice, I like it! Thanks Binh

landonreed · 2021-04-20T20:06:43Z

Commentary is a little muddy here, so I'm going to add a PR and share my recs there.

mvanlaar · 2021-04-20T20:12:32Z

@robertgregg3, what prompted the addition of the group field in gtfs.yml? I'm not sure it's needed, given that we effectively know that types 0-12 are standard and those after are extended. I liked the way you had handled it previously.

Also, Trolleybus (11) and Monorail (12) should probably be in standard and moved up in the list. These are currently shown as extended (800 and 405), but I think that's probably just because the extended list has not been updated. We could keep them both, but that might be confusing.

My opinion on this would be there will no code changes needed in the future if somethings changes to the spec. Only a edit to the gtfs.yml file. Also some companies can changes which items are for them standard and what are extended without changing code but only the gtfs.yml file.

landonreed · 2021-04-20T23:48:41Z

@robertgregg3, take a look at my changes in #659. Feel free to review and merge here if they look OK to you. And let me know if you have any questions!

robertgregg3 · 2021-04-21T13:16:04Z

@robertgregg3, what prompted the addition of the group field in gtfs.yml? I'm not sure it's needed, given that we effectively know that types 0-12 are standard and those after are extended. I liked the way you had handled it previously.

@landonreed I had an interesting time trying to create an drop down with multiple optgroups (in React). The form control in the DROPDOWN case appears to be looking for one nested div of options. When I tried to render 2 opt groups within one drop down I was getting the JSX error about it needing a surrounding div tag or React.Fragment.

When researching I cam across others with similar issues in React. I found a solution that used grouping via the options data so that is why I added the "group" to the gtfs.yml route_type options.

I did kind of prefer the other option and there was most likely a simple face palm solution.

robertgregg3 · 2021-04-21T14:01:42Z

@landonreed Thanks for sharing your solution. Looks great

robertgregg3 · 2021-04-21T14:11:06Z

@landonreed I suppose just one question regarding Evans point about updating JUST the gtfs.yml file rather than having to go into any code and updating the slice index. I prefer the code solution, but from a client usability/updating perspective perhaps the other way is more optimal?

landonreed · 2021-04-22T15:26:25Z

Re: @mvanlaar's point about avoiding a code change, yes, there may be some value in that. But these route_type values and their classification as Standard vs. Extended are intended to accurately represent the GTFS specification, rather than a third party's interpretation of what is "standard." I think we should stick with the slice method for now. Since both of these files (gtfs.yml and EditorInput.js) are committed to the repo (and there's no way to substitute alternative files for them), I don't see a major difference in the "no-code-change" (yml) vs. the js approach, so I'm inclined to stick with the current approach.

refactor(EditorInput): suggest changes for #656

binh-dam-ibigroup · 2021-09-24T16:30:19Z

This PR seemed to be blocked pending #659, but those changes have been merged since. Removing the "Blocked" label.

…value.

binh-dam-ibigroup · 2021-10-19T15:42:59Z

Per team discussion, need to add a config property to switch between the std list and the extended list.

… route types.

binh-dam-ibigroup · 2022-02-14T22:40:48Z

We can probably start reviewing this PR at this point. There may be some more refactorings possible that would touch parts that are not relevant to this PR, but let me know if you want to make them.

miles-grant-ibigroup

This actually looks quite good! The dropdown feels a bit janky but given that we're using a library this is perhaps to be expected. Other than a few style changes I think this might actually be good to go!

Thanks for updating the e2e tests as well.

miles-grant-ibigroup · 2022-02-15T17:06:59Z

lib/style.css

+
+/* Remove border and background of tags
+ * (a "tag" is a selected item displayed inside the input portion of the selector). */
+.route-type-select .dropdown-trigger .tag {


The bold looks awkward now that there is no background or border anymore. I'd either add them back (color coding might be a nice feature) or set the font-weight to match the rest of the UI

Which bold are you referring to?

Oh I see, in FF.

Thanks, addressed in bc14fed.

miles-grant-ibigroup · 2022-02-15T17:10:50Z

lib/style.css

+/* Custom classes for the DropdownTreeSelect component that
+ * override CSS from 'react-dropdown-tree-select/dist/styles.css'.
+ * Modified from https://dowjones.github.io/react-dropdown-tree-select/#/story/with-bootstrap-styles.
+ * !important tags are used because somehow the tree selector default styles are applied after this stylesheet.


We could probably clean these up by using more specific selectors, but I personally don't mind the !important too much

philip-cline

Looks good to me!

fix(EditorInput.js gtfs.yml): Added missing GTFS Extended Route Types

6236f13

Added the route types to the GTFS.yml file and then added logic to display the route_type field in an optgroup #645

robertgregg3 assigned landonreed and binh-dam-ibigroup Apr 8, 2021

landonreed reviewed Apr 8, 2021

View reviewed changes

lib/editor/components/EditorInput.js Show resolved Hide resolved

landonreed suggested changes Apr 8, 2021

View reviewed changes

landonreed assigned robertgregg3 and unassigned landonreed Apr 8, 2021

binh-dam-ibigroup reviewed Apr 9, 2021

View reviewed changes

refactor(gtfs.yml, EditorInput.js): Refactored code

07a06a6

Added a group option to the gtfs route types options. Added method to render the route type options. #656

Merge branch 'dev' into add-gtfs-extended-route-types

5289c80

landonreed reviewed Apr 20, 2021

View reviewed changes

binh-dam-ibigroup requested changes Apr 20, 2021

View reviewed changes

binh-dam-ibigroup removed their assignment Apr 20, 2021

landonreed assigned landonreed and unassigned robertgregg3 Apr 20, 2021

refactor(EditorInput): suggest changes for #656

c6989f8

landonreed mentioned this pull request Apr 20, 2021

refactor(EditorInput): suggest changes for #656 #659

Merged

landonreed added the BLOCKED Blocked (waiting on another PR to be merged) label Apr 20, 2021

Merge pull request #659 from ibi-group/add-gtfs-extended-route-types-ltr

ec0d4c1

refactor(EditorInput): suggest changes for #656

binh-dam-ibigroup added 2 commits September 24, 2021 16:30

refactor(EditorInput): Extract route type rendering, pass route type …

a86e8be

…value.

refactor(EditorInput): Reorder imports

1061f1f

binh-dam-ibigroup added 5 commits February 11, 2022 11:06

chore(gtfs.yml): Tweak route types.

9b26e23

feat(RouteTypeSelect): Add route type selector with new extended GTFS…

5509225

… route types.

Merge branch 'dev' into add-gtfs-extended-route-types

52d1c40

improvement(RouteTypeSelect): Improve rendering.

51f8848

refactor(EditorInput): Do first refactor of class.

008d7e7

binh-dam-ibigroup requested review from miles-grant-ibigroup and removed request for landonreed February 14, 2022 22:39

binh-dam-ibigroup assigned miles-grant-ibigroup and unassigned binh-dam-ibigroup Feb 14, 2022

binh-dam-ibigroup added 2 commits February 14, 2022 17:52

refactor(EditorInput): Do a light refactor.

73c48e0

test(e2e): Make e2e tests for route editing pass.

7d7928f

miles-grant-ibigroup approved these changes Feb 15, 2022

View reviewed changes

miles-grant-ibigroup assigned binh-dam-ibigroup and unassigned miles-grant-ibigroup Feb 15, 2022

refactor(lib/styles.css): Adjust styles for RouteTypeSelect.

bc14fed

binh-dam-ibigroup assigned philip-cline and unassigned binh-dam-ibigroup Feb 15, 2022

binh-dam-ibigroup requested a review from philip-cline February 15, 2022 20:29

philip-cline approved these changes Feb 16, 2022

View reviewed changes

philip-cline assigned binh-dam-ibigroup and unassigned philip-cline Feb 16, 2022

improvement(editor/util/validation): Add GTFS route type UI validation.

e78eb65

binh-dam-ibigroup merged commit 2b5bb43 into dev Feb 25, 2022

binh-dam-ibigroup deleted the add-gtfs-extended-route-types branch February 25, 2022 17:06

binh-dam-ibigroup mentioned this pull request Jun 22, 2022

Feature Release: June 22, 2022 #709

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added missing GTFS Extended Route Types #656

Added missing GTFS Extended Route Types #656

robertgregg3 commented Apr 8, 2021 •

edited by binh-dam-ibigroup

Loading

landonreed Apr 8, 2021 •

edited

Loading

robertgregg3 Apr 9, 2021

robertgregg3 Apr 9, 2021

landonreed Apr 9, 2021

landonreed Apr 8, 2021

robertgregg3 Apr 9, 2021

landonreed left a comment

binh-dam-ibigroup Apr 9, 2021

robertgregg3 Apr 9, 2021

binh-dam-ibigroup Apr 9, 2021

robertgregg3 commented Apr 20, 2021

landonreed left a comment

binh-dam-ibigroup left a comment

binh-dam-ibigroup Apr 20, 2021

binh-dam-ibigroup Apr 20, 2021

robertgregg3 Apr 21, 2021

landonreed commented Apr 20, 2021

mvanlaar commented Apr 20, 2021

landonreed commented Apr 20, 2021

robertgregg3 commented Apr 21, 2021

robertgregg3 commented Apr 21, 2021

robertgregg3 commented Apr 21, 2021

landonreed commented Apr 22, 2021

binh-dam-ibigroup commented Sep 24, 2021

binh-dam-ibigroup commented Oct 19, 2021

binh-dam-ibigroup commented Feb 14, 2022

miles-grant-ibigroup left a comment

miles-grant-ibigroup Feb 15, 2022

binh-dam-ibigroup Feb 15, 2022

binh-dam-ibigroup Feb 15, 2022

binh-dam-ibigroup Feb 15, 2022

miles-grant-ibigroup Feb 15, 2022

philip-cline left a comment

Added missing GTFS Extended Route Types #656

Added missing GTFS Extended Route Types #656

Conversation

robertgregg3 commented Apr 8, 2021 • edited by binh-dam-ibigroup Loading

landonreed Apr 8, 2021 • edited Loading

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

landonreed left a 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

robertgregg3 commented Apr 20, 2021

I have added:

landonreed left a comment

Choose a reason for hiding this comment

binh-dam-ibigroup left a 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

landonreed commented Apr 20, 2021

mvanlaar commented Apr 20, 2021

landonreed commented Apr 20, 2021

robertgregg3 commented Apr 21, 2021

robertgregg3 commented Apr 21, 2021

robertgregg3 commented Apr 21, 2021

landonreed commented Apr 22, 2021

binh-dam-ibigroup commented Sep 24, 2021

binh-dam-ibigroup commented Oct 19, 2021

binh-dam-ibigroup commented Feb 14, 2022

miles-grant-ibigroup left a 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

philip-cline left a comment

Choose a reason for hiding this comment

robertgregg3 commented Apr 8, 2021 •

edited by binh-dam-ibigroup

Loading

landonreed Apr 8, 2021 •

edited

Loading