refactor: Improve GL item config types #2138
Conversation
| @@ -66,7 +66,7 @@ export default class Component extends AbstractContentItem { | |||
| } | |||
|
|
|||
| close() { | |||
| this.parent.removeChild(this); | |||
| this.parent?.removeChild(this); | |||
There was a problem hiding this comment.
Trying to wrap my head around how this didn't cause problems before, but e2e tests would suggest that Component item types can be instantiated without a parent.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2138 +/- ##
==========================================
+ Coverage 46.59% 46.62% +0.03%
==========================================
Files 682 685 +3
Lines 38441 38492 +51
Branches 9578 9800 +222
==========================================
+ Hits 17912 17948 +36
+ Misses 20519 20492 -27
- Partials 10 52 +42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -276,8 +266,9 @@ export default class Stack extends AbstractContentItem { | |||
| const insertBefore = | |||
| this._dropSegment === 'top' || this._dropSegment === 'left'; | |||
| const hasCorrectParent = | |||
| (isVertical && this.parent.isColumn) || | |||
| (isHorizontal && this.parent.isRow); | |||
| this.parent instanceof RowOrColumn && | |||
There was a problem hiding this comment.
I am making a slight assumption adding the instanceof RowOrColumn check here, since AbstractContentItem has an optional isColumn and isRow prop. Technically it wouldn't have to be said instance to have passed this check in the past. The other piece is the call to addChild on line 200 which does assume the 3rd argument in addChild which is only present in RowOrColumn, so it seems reasonable to think this should always be an instance of RowOrcolumn
| type ItemConfigSupported = | ||
| | ColumnItemConfig | ||
| | RowItemConfig | ||
| | StackItemConfig | ||
| | ComponentConfig; | ||
|
|
||
| const SUPPORTED_ITEM_TYPES = ['column', 'row', 'stack', 'component'] as const; |
There was a problem hiding this comment.
Docstrings for why these are the only supported types? Why isn't ReactComponentConfig supported for example? What is this "supported" by? We use ReactComponentConfig in a lot of places elsewhere... Maybe a better name would be something like LayoutItemConfig ?
There was a problem hiding this comment.
@bender I don't actually know the answer as to why we only support these. This was just based on the constructors that were in the previous code. Any ideas?
There was a problem hiding this comment.
I guess the ReactComponentConfig isn't listed due to it being converted out where this is used. I can improve that in the names / docs here to be more explicit, but not sure of any other reasons why other types aren't supported.
There was a problem hiding this comment.
Updated based on what I know. Not sure why DefaultItemConfig and RootItemConfig aren't included, but should at least clarify why ReactComponentConfig is excluded
ItemConfigsub types and convertedItemConfigtype into a union them allcreateContentItemto be more explicit in its mapping of config types to returned content itemsItemConfigTypewithItemConfigComponent,RowOrColumn, andStackclasses to allow nullparentresolves #2130