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 Closures list #2288

Merged
merged 42 commits into from
Nov 15, 2024
Merged

feat: Elevator Closures list #2288

merged 42 commits into from
Nov 15, 2024

Conversation

cmaddox5
Copy link
Contributor

@cmaddox5 cmaddox5 commented Nov 5, 2024

This PR adds a simple list view of elevator closures throughout the system. It works very similarly to Pre-Fares except all dynamic paging is done on the client. The way I handled paging was to generate the pages before React drew anything on the screen and then switch to rendering these pages once it is done. This appears seamless and looks like it loads like any other screen. After all pages have been rendered, it executes a callback that tells React it is ready to replace it's data in the next refresh. The way I did this in code feels a little messy so please do suggest improvements.

@digitalcora
Copy link
Contributor

digitalcora commented Nov 5, 2024

The client-side resizing approach here seems much more complicated than it needs to be — I thought we had been on the same page about how to implement this, so either that's my bad for not being clear enough, or maybe you ran into some issues?

Basically my idea was that Flexbox can almost entirely do the work for us. Throw this into a sandbox:

<div class="container">
  <div class="closures">
    <div class="closure">
      Repellendus nisi ut adipisci aperiam enim. Itaque quam recusandae voluptatem eum asperiores mollitia.
    </div>
    <div class="closure">
      Ipsum ea et quidem. Nam quidem aut voluptas. Sed qui at quis doloribus. Facere neque voluptatum dolor temporibus a aliquam. Soluta aut eaque ipsam doloremque. Possimus accusantium occaecati reiciendis expedita quia.
    </div>
    <div class="closure">
      Itaque recusandae adipisci est harum possimus exercitationem. Odit voluptas consequatur tempore deserunt et officia. Quas tenetur quia veniam dolores vero.
    </div>
    <div class="closure">
      Distinctio distinctio voluptates quo. Recusandae non est natus ex velit dolores beatae. Est nisi sit nobis corrupti tempora. Debitis et autem voluptatem reprehenderit cum. Occaecati tempore aut praesentium mollitia omnis perferendis nam.
    </div>
    <div class="closure">
      In dicta ut explicabo aliquam distinctio. Et et neque et. Est architecto quaerat debitis.
    </div>
  </div>
</div>
.container {
  width: 300px;
  border: 1px dotted black;
  overflow: hidden;
}

.closures {
  height: 200px;
  display: flex;
  flex-direction: column;
  flex-wrap: wrap;
}

.closure {
  border: 1px solid gray;
}
const closures = document.getElementsByClassName("closure");

for (const closure of closures) {
  console.log(closure.offsetLeft);
}

And you'll get this (I commented out overflow: hidden so we can see what's "behind the overflow"):

Screenshot 2024-11-05 at 3 49 34 PM

Without any script, this already lays out how we want: when a closure is too tall to fit on the current page, it flexes to the next page (i.e. column). The script is demonstrating that by collecting the values of offsetLeft, we can both tell how many pages there are and how many items are on each page (we'd do this inside a useLayoutEffect). Then the only missing piece is how to display a specific "page" other than the first one, which we already have a pretty clean example of in LaterDepartures that uses a CSS variable.

Let me know if that makes sense, or if you already tried this and it didn't work.

assets/css/v2/elevator/elevator_closures.scss Outdated Show resolved Hide resolved
assets/src/components/v2/elevator/elevator_closures.tsx Outdated Show resolved Hide resolved
assets/src/components/v2/elevator/elevator_closures.tsx Outdated Show resolved Hide resolved
assets/src/components/v2/elevator/elevator_closures.tsx Outdated Show resolved Hide resolved
lib/screens/v2/candidate_generator/elevator/closures.ex Outdated Show resolved Hide resolved
lib/screens/v2/candidate_generator/elevator/closures.ex Outdated Show resolved Hide resolved
lib/screens/v2/candidate_generator/elevator/closures.ex Outdated Show resolved Hide resolved
@@ -38,7 +38,7 @@ defmodule Screens.V2.ScreenData.Parameters do
},
elevator_v2: %Static{
candidate_generator: CandidateGenerator.Elevator,
refresh_rate: 30
refresh_rate: 8
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, is our "persistent" component logic actually tied to the data refresh rate? That seems less than ideal, but maybe just a refactoring opportunity to keep in mind for the future. (Suppose your screen consists almost entirely of a paging component, and you expect it to normally have at least two pages: it can only "really" refresh once all the paging is done, so you're doing at least twice as many data refreshes as you need to.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does currently, yes, and I just felt like it would be too far out of scope to adjust it here. Given that data refreshes are built into the framework, we may need to get around it by preventing it from refreshing at this level and adding data fetches to the widget component itself. Then where we call onFinish() currently could be replaced by that fetch.

lib/screens/v2/widget_instance/elevator_closures.ex Outdated Show resolved Hide resolved
@cmaddox5 cmaddox5 force-pushed the cm/list-elevator-closures branch from 393a468 to 8b4c7d5 Compare November 8, 2024 14:18
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.

Looking pretty good! A few more minor comments.

assets/src/components/v2/elevator/elevator_closures.tsx Outdated Show resolved Hide resolved
lib/screens/v2/candidate_generator/elevator/closures.ex Outdated Show resolved Hide resolved
assets/src/components/v2/elevator/elevator_closures.tsx Outdated Show resolved Hide resolved
lib/screens/v2/location_context.ex Outdated Show resolved Hide resolved
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.

A couple (really one) small comments, but I say this looks ready to go!

}) do
facility = Enum.find_value(entities, fn %{facility: facility} -> facility end)

%{
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we've defined ElevatorClosures.Closure we could use it here instead of a plain map.

|> Map.fetch!(parent_station_id)
|> Enum.map(&RoutePill.serialize_icon/1)

%{
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, with ElevatorClosures.Station.

@cmaddox5
Copy link
Contributor Author

@digitalcora Just want to point out that I also tweaked the CSS so it uses compatible features. As of last night, the office screen now renders how we expect it to:
IMG_7835

@cmaddox5 cmaddox5 merged commit 452160d into main Nov 15, 2024
11 checks passed
@cmaddox5 cmaddox5 deleted the cm/list-elevator-closures branch November 15, 2024 20:01
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