rdkcentral
fix(FocusManager): add style TS definitions for FocusManager, NavigationManager, Row, and Column
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
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.
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
