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: Elevator Screens #529

Merged
merged 9 commits into from
Oct 25, 2024
Merged

feat: Elevator Screens #529

merged 9 commits into from
Oct 25, 2024

Conversation

cmaddox5
Copy link
Contributor

@cmaddox5 cmaddox5 commented Oct 17, 2024

Now that we can make elevator screens, we need to make them work in Screenplay. This involves a few things:

  • Adding a way to determine stop_id in Builder. Did this with a /facilities lookup
  • Correcting the ID in some existing client code (was added a long time ago but we guessed ID incorrectly)
  • Add styles so iframes display correctly

Deploying this to dev-green so it can be seen in a deployed environment.


@cmaddox5 cmaddox5 requested a review from a team as a code owner October 17, 2024 13:47
assets/js/components/Dashboard/PlaceRowAccordion.tsx Outdated Show resolved Hide resolved
lib/screenplay/places/builder.ex Outdated Show resolved Hide resolved
test/fixtures/builder/screens_config.json Outdated Show resolved Hide resolved
@cmaddox5 cmaddox5 force-pushed the cm/support-elevator-screens branch from ef230bd to dd5effe Compare October 23, 2024 12:39
@cmaddox5 cmaddox5 requested a review from digitalcora October 23, 2024 12:41
Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

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

One comment, looks good otherwise!

@@ -15,20 +15,22 @@ import ScreenDetail from "Components/ScreenDetail";
import { sortScreens } from "../../util";
import { useUpdateAnimation } from "Hooks/useUpdateAnimation";
import classNames from "classnames";
import _ from "lodash";
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we were using lodash/fp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yes thank you. (Although I dislike how bad it is at inferring types 😅)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this is a notable downside of FP and maybe something we should have considered (if we'd had a better sense for it at the time) in our conversations about which "utility library" to use. I tend to avoid Lodash entirely and use vanilla JS where feasible, since built-in classes and functions have generally excellent types (except for you, JSON.parse, you're not invited to the party).

@cmaddox5 cmaddox5 merged commit 978c86c into main Oct 25, 2024
2 checks passed
@cmaddox5 cmaddox5 deleted the cm/support-elevator-screens branch October 25, 2024 15:22
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.

2 participants