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

Support Exception Based Scheduling #999

Merged
merged 6 commits into from
Nov 30, 2023

Conversation

philip-cline
Copy link
Contributor

@philip-cline philip-cline commented Nov 9, 2023

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JSDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

This PR provides client support for exception based scheduling. To create an exception based schedule, either import a feed with calendar_dates based service or create a new exception of the new "Exception Based Service" type (or both!).

Relies on:
ibi-group/datatools-server#545
conveyal/gtfs-lib#385

@philip-cline philip-cline removed the WIP label Nov 21, 2023
Copy link
Contributor

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Big lift, thanks for the effort! Looks good, just a few sanity checks

lib/editor/actions/editor.js Show resolved Hide resolved
</BsLabel>
</>
: <>
<Icon type='warning' />(Exception based calendar)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this look good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment is directed at you.
image
I'm not sure I ever rly thought that would be a permanent look, I'm open to suggestions. I think something should probably identify it as an exception based service in the CalendarSelect

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a space between the icon and the text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, yes we can

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can even do some i18n in 369f85f

Copy link
Contributor

Choose a reason for hiding this comment

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

To be brief, we could say [Name] (Exception)
Or is Exception based calendar a commonly used term?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Exception based calendar is more clear for the user. We can maybe wait for some feedback if people find it cumbersome

lib/editor/components/timetable/CalendarSelect.js Outdated Show resolved Hide resolved
Copy link
Contributor

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Tested to the extent I understand what is going on

Copy link
Contributor

@AdrianaCeric AdrianaCeric left a comment

Choose a reason for hiding this comment

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

This looks good! The UI makes sense when testing some cases. Feel free to address the few validation things we noticed

Comment on lines +63 to +66
<BsLabel
title={`Calendar has trips for ${routeCount} routes`}>
<Icon type='bus' /> {routeCount}
</BsLabel> **/}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, that's historical. But it seems at one point we displayed the number of trips for a calendar by routes, rather than patterns.

@philip-cline philip-cline merged commit 90eef44 into dev Nov 30, 2023
5 checks passed
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.

None yet

3 participants