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

feat: add fields for V2 Busway screens #19

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Conversation

digitalcora
Copy link
Contributor

@digitalcora digitalcora commented Apr 2, 2024

Asana: https://app.asana.com/0/1185117109217413/1206932080790885

This is intended to account for the full range of configuration possible for v1 Solari screens, which is documented here. Reviewers should hold me to that 😄

Things to note:

  • I've added some documentation to the modules I touched, in an attempt to start writing down our expectations of how the configuration should make signs behave. Ideally this is not too detailed (it's not an implementation guide), or there will be drift between the documentation and reality if we change how an existing field is interpreted and forget to update the docs here.

  • Per discussion with @jzimbel-mbta, the pill field from the v1 config is omitted, as this information can be derived from the other fields (and DUP displays already do this, so we would not need to write the logic from scratch).

  • The filter field of Section does not seem to be used in any active configuration, and was probably only added to accommodate busway screens in the future. Because of this I thought it should be okay to rename this to filters, nest the route-direction filtering inside it, and then that allows us to add the "minutes" filtering in a logical place. I'm not sure if changing the existing structure like this actually works, though (I expect it to quietly drop the old field the next time the configuration is serialized, but if that's not the case, this might need to change.)

@digitalcora digitalcora force-pushed the cfg-busway-v2-fields branch from c864bbc to 310019d Compare April 2, 2024 21:57
@digitalcora digitalcora requested review from a team and cmaddox5 and removed request for a team April 3, 2024 15:40
@digitalcora digitalcora marked this pull request as ready for review April 3, 2024 15:40
@jzimbel-mbta
Copy link
Member

jzimbel-mbta commented Apr 5, 2024

(Just swooping in to set Christian as the assignee—we have a convention of using the assignee field to indicate whose court the ball is currently in, so to speak. I personally use https://github.com/pulls/assigned when looking for stuff to review, so if it's not set I'm likely to miss the PR! Not sure if Christian does the same thing.)

edit: lol you just did it nvm!

Copy link
Contributor

@cmaddox5 cmaddox5 left a comment

Choose a reason for hiding this comment

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

Looks great! The moduledocs are very clear and answered any question I had while reviewing.

@cmaddox5 cmaddox5 assigned digitalcora and unassigned cmaddox5 Apr 5, 2024
@digitalcora digitalcora force-pushed the cfg-busway-v2-fields branch from 41a5ef7 to 0961f61 Compare April 5, 2024 18:21
@digitalcora digitalcora merged commit f5d84ae into main Apr 5, 2024
2 checks passed
@digitalcora digitalcora deleted the cfg-busway-v2-fields branch April 5, 2024 18:23
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.

3 participants