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: departures multi-section auto-sizing #2048

Merged
merged 3 commits into from
May 20, 2024
Merged

Conversation

digitalcora
Copy link
Contributor

@digitalcora digitalcora commented May 16, 2024

Best reviewed per-commit, there are a couple independent refactors.

  • Update departures widget generation to pass layout parameters from configuration to the frontend.

  • Replace existing auto-sizing implementation, which only worked for a single section, with a version that handles multiple sections using the layout parameters.

  • The auto-sizing itself (independent of the React component) is unit-tested. These are the first real frontend tests in Screens, so we take the opportunity to:

    • Update Jest to its latest version.

    • Replace the third-party @types/jest package with a direct dependency on @jest/globals, which is already part of Jest. The only advantage of the former is that it allows using describe, test, etc. without importing them.

    • Add eslint-plugin-jest to our ESLint setup.

    • Add fishery for defining factories to build test data.


Pass individual sections to the post-processing function instead of the
entire list of sections. The only current use of post-processing is in
the `GlEink` screen type, which only supports one section, so there is
no requirement to be able to access all sections at once.
@digitalcora digitalcora force-pushed the cfg-section-sizing branch 2 times, most recently from 6952148 to de3849a Compare May 16, 2024 19:18
@mbta mbta deleted a comment from github-actions bot May 16, 2024
@digitalcora digitalcora marked this pull request as ready for review May 16, 2024 19:54
@@ -37,7 +37,7 @@ interface TimestampDeparture {
minute: number;
}

const DepartureTime = ({ type, ...data }: Props) => {
const DepartureTime: ComponentType<DepartureTime> = ({ type, ...data }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

quibble (non-blocking): i'm hesitant to rely on typescript's separate namespaces for types and values in this way. could we call one of these DepartureTime things something else?

update: i see this is pattern is used a lot throughout this commit. if this is already an established pattern elsewhere please feel free to disregard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used this pattern in places where a distinct "object" returned by the server corresponds exactly to the props of the component which renders an instance of that object. A DepartureTime (value) is a component which renders a DepartureTime (type). Within their namespaces, I think it makes sense to call each one of these by that name. But things don't always line up this way, as with e.g. the component one level above this, DepartureTimes: in this case the "props type" has a distinct name and is not exported.

export type TimeWithCrowding = { /* ... */ };

type Props = { timesWithCrowding: TimeWithCrowding[] };

const DepartureTimes: ComponentType<Props> = ({ timesWithCrowding }) => { /* ... */ };

I'd hesitate to say there's an "established pattern", since unfortunately a lot of the existing codebase just doesn't have explicit types at all (which is why we currently allow implicit-any). If anything it currently leans towards using separately-named "props types" in all cases. So I guess I'm proposing that we establish this as a pattern 🙂 What do you think?

In any case, thanks for calling this out, since I realized we should be using import type in places where we only use the type. It looks like the linter has a rule to enforce this but it's off by default. I'll see what happens when we turn it on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, the lint rule gives 162 errors on the current codebase... it looks like it's less sophisticated than I thought, and just errors whenever you import a type without using type, even if the name has no overlap with a value. (Though it seems like this could have advantages for bundle size anyway, so maybe we should think about turning it on, separately?)

I looked at all the imports across changed files in this PR, but as far as I can see, there actually isn't any place where I imported a value+type name and only used the type, so I haven't added any type keywords here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If anything it currently leans towards using separately-named "props types" in all cases. So I guess I'm proposing that we establish this as a pattern 🙂 What do you think?

This is my personal preference so I'm on-board!

As an aside, I also prefer to use something like zod to parse external data to ensure that types for it are accurate and not just wishful thinking. 📌 We can put a pin in that for now though.

assets/src/components/v2/departures/departure_row.tsx Outdated Show resolved Hide resolved
assets/src/components/v2/departures/section.ts Outdated Show resolved Hide resolved
assets/src/components/v2/departures/section.ts Outdated Show resolved Hide resolved
@mbta mbta deleted a comment from github-actions bot May 17, 2024
Copy link
Contributor

@sloanelybutsurely sloanelybutsurely left a comment

Choose a reason for hiding this comment

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

LGTM (as far as I can tell that is...). It might still be a good idea to have someone else take a look because I'm really just looking at code with little context. I've made very little judgement based on behavior here.

We can type names as-is and have that discussion more broadly whenever we feel like it ("never" also an option for when to chat about it)

@mbta mbta deleted a comment from github-actions bot May 20, 2024
Copy link
Contributor

@cmaddox5 cmaddox5 left a comment

Choose a reason for hiding this comment

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

Really digging the approach here. And the code is organized in a way that makes this complicated operation very approachable.

* Update departures widget generation to pass layout parameters from
  configuration to the frontend.

* Replace existing auto-sizing implementation, which only worked for a
  single section, with a version that handles multiple sections using
  the layout parameters.

* The auto-sizing itself (independent of the React component) is
  unit-tested. These are the first real frontend tests in Screens, so
  we take the opportunity to:

  * Update Jest to its latest version.

  * Replace the third-party `@types/jest` package with a direct
    dependency on `@jest/globals`, which is already part of Jest. The
    only advantage of the former is that it allows using `describe`,
    `test`, etc. without importing them.

  * Add `eslint-plugin-jest` to our ESLint setup.

  * Add `fishery` for defining factories to build test data.
Copy link

Coverage of commit 683ddd1

Summary coverage rate:
  lines......: 44.5% (2845 of 6394 lines)
  functions..: 42.0% (1019 of 2428 functions)
  branches...: no data found

Files changed coverage rate:
                                                                          |Lines       |Functions  |Branches    
  Filename                                                                |Rate     Num|Rate    Num|Rate     Num
  ==============================================================================================================
  lib/screens/v2/candidate_generator/dup/departures.ex                    |88.2%    136|77.3%    22|    -      0
  lib/screens/v2/candidate_generator/gl_eink.ex                           |17.4%     46|16.7%    18|    -      0
  lib/screens/v2/candidate_generator/widgets/departures.ex                |90.2%     41|84.6%    13|    -      0
  lib/screens/v2/widget_instance/departures.ex                            |81.0%    142|81.8%    33|    -      0

Download coverage report

@mbta mbta deleted a comment from github-actions bot May 20, 2024
@digitalcora digitalcora merged commit 6593dd0 into main May 20, 2024
9 checks passed
@digitalcora digitalcora deleted the cfg-section-sizing branch May 20, 2024 17:34
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.

3 participants