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

fix(FocusManager): add style TS definitions for FocusManager, NavigationManager, Row, and Column #411

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

erautenberg
Copy link
Contributor

Description

In building the new Gallery component, I tried adding his to the Gallery.d.ts file:

type GalleryStyle = ColumnStyle & {
  itemLayout?: LayoutOptions;
};

I found that ColumnStyle doesn't actually exist. While that makes sense since there are no Column-specific style properties compared to NavigationManager, I'm thinking Row and Column should export their own style types even if they're empty, just so that we future-proof ourselves for the amount of inheritance we have.

This turned into a down the rabbit hole situation where a bunch of components need style types exported too, so I was thinking even Base could have some styles (otherwise I can remove that and just start the process at FocusManager as FocusManagerStyles = object).

Automation

Checklist

  • all commented code has been removed
  • any new console issues have been resolved
  • code linter and formatter has been run
  • test coverage meets repo requirements
  • PR name matches the expected semantic-commit syntax

@erautenberg erautenberg added the WIP In progress but could use an initial review label Nov 15, 2023
@svc-lightning-ui-components
Copy link
Collaborator

Test Execution Failed.

1 similar comment
@svc-lightning-ui-components
Copy link
Collaborator

Test Execution Failed.

@ImCoolNowRight
Copy link
Contributor

To add a comment instead of just approving: I agree with this approach and think that it makes sense to define styles at each level, even if they are exactly the same as their parent class. There may not be a huge benefit upfront, but I do think it solves exactly the issue that you ran into, where you have to work backwards when you suddenly need to define styles for an object and the parent doesn't have them defined. I also think this makes it abundantly clear that the styles are identical to the parent's.

@erautenberg erautenberg added DO NOT MERGE and removed WIP In progress but could use an initial review labels Nov 22, 2023
@anthony9187
Copy link
Contributor

anthony9187 commented Nov 22, 2023

If everyone agrees on this path forward I'm not going to block it but I'd like to put my thoughts here:

We went through a similar decision with the TypeConfigs and TemplateSpecs for each component - we had the option to explicitly declare a duplicated TypeConfig and TemplateSpec for each component, or reference the exact one(could be a bit further up the inheritance chain than the parent) we were implementing. We decided it was better to reference the exact interface instead of redeclaring empty interfaces because 1. TS just doesn't really like it and 2. It becomes less clear what's actually being referenced. So if you're trying to get a look at the interface when digging through the source, you wind up having to click your way up through type definitions until you get to the actual one. It also makes the inheritance chain more explicit - I know which components leverage the same interfaces just by looking at them instead of having to search my way up the chain until I hit a real interface.

In general, my preference has been to let things be what they are instead of consistently re-mapping them under a new name

@@ -18,6 +18,8 @@

import lng from '@lightningjs/core';

type BaseStyle = object;
Copy link
Contributor

@anthony9187 anthony9187 Nov 22, 2023

Choose a reason for hiding this comment

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

this works, but we might be able to use a more explicit type here

If you want a type meaning "empty object", you probably want Record<string, never> instead.
If you want a type meaning "any object", you probably want object instead.
If you want a type meaning "any value", you probably want unknown instead.
^ https://stackoverflow.com/a/75753960

I think Record<string, never> is the most semantically correct, but I also think it could break the way we do style types. so object might be fine. just a thought

Copy link
Contributor

Choose a reason for hiding this comment

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

Would Record<String, never> force the style to be flat, or could it still have nested objects?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants