-
Notifications
You must be signed in to change notification settings - Fork 2
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: Current elevator closure layout #2315
Conversation
3d38319
to
3f58dc3
Compare
3f58dc3
to
f72c504
Compare
32b20af
to
eecb902
Compare
eecb902
to
5db01b9
Compare
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.
Approving with one suggestion, take it if you'd like — this is now over a week old (though I know holidays etc.) and seems in good shape, so I don't want to hold it up any further!
lastUpdate={lastUpdate} | ||
onFinish={onFinish} | ||
/> | ||
{currentElevatorClosure ? ( |
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.
We might have already talked about this, but I think it could be worthwhile to move this conditional to the widget generation. This single "widget" is basically two widgets in a trenchcoat — we always send over all the fields for both, and then throw half of them away, depending on a condition the server itself could easily check. In other words, we could instead have a CurrentElevatorClosed
widget and an OutsideClosures
widget on the backend, each populated with only the fields needed for that "view".
I could be missing something about future plans for this widget that might require it to have access to both sets of information at the same time, though.
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.
Big fan of that approach. That would likely be a pretty big diff though. You think it makes sense here or should I create a followup PR?
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 think within this PR is actually the best place for it, since this is where we're introducing the second widget. Else we'd have (in the final commits on main
) one commit that adds the widget using this approach, and a second that completely changes the approach. That's a lot of extra, and ultimately pointless, information for anyone looking at history to sort through.
I'm happy to quickly re-review the change if you want one more look at it.
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.
Should be good to go if you want to take a look.
f7ac378
to
627f758
Compare
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.
Looking good!
end | ||
|
||
describe "serialize/1" do | ||
test "returns map with id, closure, and alternate direction info", %{instance: instance} do |
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 don't know if we really need all these "it does the thing" tests for functions whose implementation is extremely basic... ideally the code paths would be covered by some higher-level test, but we have no such category of tests at the moment. Guess I won't make any specific recommendation here, just noting that it's an unfortunate situation.
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 have this thought every time I make tests for a new widget. Definitely a good idea to put end-to-end framework tests on some kind of radar. Not sure if that's big enough to be roadmap worthy or if a simple tech-debt task would be sufficient?
This PR adds a new layout for elevator screens. When the current elevator is closed at all, we show this layout that helps riders get to an alternate elevator. Maps will be created by Betsy and added to S3. Coordinates for the map marker will also be provided by Betsy.
Config that can be used for testing: