-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: Add new fields to elevator widget #44
Conversation
lib/config/v2/elevator.ex
Outdated
evergreen_content: list(EvergreenContentItem.t()), | ||
alternate_direction_text: String.t(), | ||
accessible_path_image_url: String.t(), | ||
accessible_path_direction_arrow: arrow_direction() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since none of these are nullable and all are enforced when constructing, are we fine with manually updating existing elevator screen configs? Just calling out that we'd have to remember to do this (and either have content ready to go or use "stub" content).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can go ahead and stub them out. I think it's important for these to be required in the future because without them, the screens would be mostly blank if the current elevator is closed.
lib/config/v2/elevator.ex
Outdated
@@ -3,14 +3,23 @@ defmodule ScreensConfig.V2.Elevator do | |||
|
|||
alias ScreensConfig.V2.EvergreenContentItem | |||
|
|||
@type arrow_direction :: :n | :s | :e | :w |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to support diagonal arrows? There's precedent for this (Sectional header wayfinding), so I don't think it would be that difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can ask Betsy that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. We want diagonal as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. It would be nice if we could unify the types, then — maybe have a ScreensConfig.Arrow.t()
referenced by both. (or maybe it's too early for that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. There were a few other configs that supported this type so I changed those as well.
17eb9c7
to
a417acd
Compare
a417acd
to
33d67b5
Compare
Adding some new fields that will be needed when building out the "Current elevator closed" view on the widget.