feat: Working triptych app skeleton with viewport shifter, request logging, packaging instructions #1833
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jzimbel-mbta
changed the title
jzimbel-mbta
commented
Aug 11, 2023
Coverage of commit
|
Coverage of commit
|
jzimbel-mbta
commented
Aug 11, 2023
jzimbel-mbta
commented
Aug 11, 2023
jzimbel-mbta
commented
Aug 11, 2023
jzimbel-mbta
commented
Aug 11, 2023
jzimbel-mbta
commented
Aug 11, 2023
There was a problem hiding this comment.
A lot of the layout components + CSS were stubbed out, which I didn't expect. I knew we'll need to do them eventually, but I figured the packaging step was primarily to get the bare bones app exported, even if it was just the blank screen with slot_names. (But perhaps Paul expected the placeholders like you did. Maybe a boring screen would have been a waste of Outfront's time.) Couple notes to get us started, while Outfront looks at the app
Coverage of commit
|
cmaddox5
reviewed
Aug 14, 2023
Coverage of commit
|
| @@ -37,7 +37,7 @@ const ScreenPage = ({ | |||
| }: { | |||
| screenContainer: React.ComponentType; | |||
| }): JSX.Element => | |||
| isDup() ? ( | |||
| isDUP() ? ( | |||
There was a problem hiding this comment.
I have mixed feelings about updating anything for DUP v1 (though maybe this update was needed to compile?)
There was a problem hiding this comment.
Yeah, my bad, this is an artifact of my initially merging the check into a new isOFM function, and then needing to go back and have separate isDUP and isTriptych functions. In the process I accidentally renamed the original isDup to isDUP. 😵💫
I'll restore this function's original name.
There was a problem hiding this comment.
I checked the latest updates, and it still looks like the changes are there, so we might be talking about different things.
Since this is a DUP v1 file, I'm wondering why the change to the function name needed to be made? We don't have any DUP v1s in the world anymore and I'm not concerned about keeping it maintained, but maybe that's a philosophically bad approach?
Coverage of commit
|
Coverage of commit
|
…ier making a mess of the existing code)
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
hannahpurcell
approved these changes
Aug 30, 2023
Comment on lines
+37
to
+40
| // No-data states | ||
| @import "v2/lcd_common_styles/page_load_no_data"; | ||
| @import "v2/lcd_common_styles/no_data"; | ||
| @import "v2/triptych/no_data"; |
There was a problem hiding this comment.
I'll try to get this reviewed / merged before the no-data branch. It's possible these may / may not be needed depending on the implementation of that, but we can leave that for the no-data pr to resolve.
| @@ -37,7 +37,7 @@ const ScreenPage = ({ | |||
| }: { | |||
| screenContainer: React.ComponentType; | |||
| }): JSX.Element => | |||
| isDup() ? ( | |||
| isDUP() ? ( | |||
There was a problem hiding this comment.
I checked the latest updates, and it still looks like the changes are there, so we might be talking about different things.
Since this is a DUP v1 file, I'm wondering why the change to the function name needed to be made? We don't have any DUP v1s in the world anymore and I'm not concerned about keeping it maintained, but maybe that's a philosophically bad approach?
| @@ -17,7 +17,7 @@ import { | |||
| MultiRotationPage, | |||
| SimulationPage, | |||
| } from "Components/dup/dup_screen_page"; | |||
| import { isDup } from "Util/util"; | |||
| import { isDup } from "Util/outfront"; | |||
There was a problem hiding this comment.
I unresolved a previous comment thread to ask about this in better detail. (Just noting here so you don't miss it.)
There was a problem hiding this comment.
Yeah, if I hadn't updated this and other references to isDup in the old code, I think the code would fail to compile or at least print some warnings to the console even though we don't have any screens configured to use that app bundle anymore.
The real reason it's updated though, is simply that it's easier to do so. I used the vscode refactor function (F2) to rename the function as well as automatically update all references to it. It would have been harder to go back afterward and undo the renames in the old files.
There was a problem hiding this comment.
The real reason it's updated though, is simply that it's easier to do so. I used the vscode refactor function (F2)
Love it! That's a great reason.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Asana task: Get basic triptych app working & set up packaging for OF process
This ended up being a bit of a snowball of changes—I needed to add some new frontend and backend code to support how we plan to identify individual triptych panes based on their player names.
The diff is huge, but it's almost all from straightforward refactors I did to make working on the OFM client apps less error-prone.
Summary of changes since the first round of reviews
Triptych player name mapping config
The server now expects us to maintain a mapping from triptych player name to screen ID in a config file.
Implementation is identical to that of the main screen config—we use a local JSON file in priv/ for local development, and per-environment files in S3 for deployment environments. The mapping is cached on the server, and can be accessed with functions in
Screens.TriptychPlayer.State.This required a terraform change to add permission for the new file in S3. Link to relevant PR
Triptych API endpoint
A new API endpoint:
/v2/screen/:player_name/triptych.The triptych client app uses this endpoint when running in a standalone package.
It maps the player name to a screen ID using the new config, then runs identical logic to all other screen types.
Determine pane from MRAID tags
The triptych client now determines which pane of the trio it is via the
Array_configurationMRAID tag.☝️ ^ The above 3 changes ^ collectively mean that we only need to package one triptych app now! (Not 3)
Improved common OFM functions
Utility functions related to OFM stuff have been collected in Util/outfront.tsx and Hooks/outfront.tsx, and cleaned up.
The OFM hooks no longer use state or effects—just a memoized function call. This is because the values in the MRAID object are not expected to change. By using
useMemoinstead ofuseState+useEffect, we don't need to worry about handling an "initial" value likenullon the first render, as well as the real value in subsequent renders.useMemoreturns the real value synchronously on the first render.Package testing improvements
Fake MRAID object
The process for testing packaged DUP/triptych apps locally is now closer to how they work on real OFM screens.
You can call a function to set up a fake MRAID object that matches the interface of the real one.
Improved debug element
Improved the on-screen debug element for OFM clients. Newest lines now appear first so that relevant info doesn't scroll out of view after the first couple renders.
Screenshots
left
middle
right