chore(ui5-table): remove selection import & rename key #9300
Conversation
DonkeyCo
changed the title
| @@ -17,9 +17,14 @@ const findRowInPath = (composedPath: Array<EventTarget>) => { | |||
| return composedPath.find((el: EventTarget) => el instanceof HTMLElement && el.hasAttribute("ui5-table-row")) as TableRow; | |||
| }; | |||
|
|
|||
| const isFeature = <T>(element: any, identifier: string): element is T => { | |||
There was a problem hiding this comment.
Probably we can integrate the existing mechanism
// TableSelection.ts
import { registerFeature } from "@ui5/webcomponents-base/dist/FeaturesRegistry.js";
registerFeature("TableSelection", TableSelection);When someone imports the TableSelection.ts module, the feature is registered, then the Table can call getFeature to check if the feature is enabeled:
import { getFeature } from "@ui5/webcomponents-base/dist/FeaturesRegistry.js";
const TableSelection = getFeature<typeof TableSelection>("TableSelection");See Input.ts, InputSuggestions.ts for reference, the suggestions are like plug-in feature on top, don't come built in with the Input by default but only when someone imports the InputSuggestions module explicitly.
This is I guess the same with the Table and TableSelection.
There was a problem hiding this comment.
Is it the same though?
In case of Input.ts and InputSuggestions.ts, when the import for InputSuggestions exist, the Input will create a new instance of it.
While in the Table.ts, even if you have TableSelection imported, you need to explicitly slot it into features slot of the table. This would allow you to have multiple tables with one having selection and the other not.
So even if I check whether the feature has been imported:
// Table.ts
const TableSelection = getFeature<typeof TableSelection>("TableSelection");
if (!TableSelection) { return; }
...I still need to check, if the features slot contains a feature, that is of instance TableSelection like:
// Table.ts
const TableSelection = getFeature<typeof TableSelection>("TableSelection");
if (!TableSelection) { return; }
this.features.find(feature => feature instanceof TableSelection);So there is no real benefit from registering the feature, isn't it?
This isFeature check still needs to be done to 100% determine that the feature is of type TableSelection (as to avoid this instanceof check)
So as far as I understand, this feature registration is quite nice for features that are then created by the component themselves (e.g. Input creates a new instance of InputSuggestion), but not really applicable, if the "feature" comes from the application.
Or am I misunderstanding something here?
| @@ -26,7 +26,7 @@ import { isSelectionCheckbox, isHeaderSelector, findRowInPath } from "./TableUti | |||
| * * Multiple - select multiple rows. | |||
| * * None - no selection active. | |||
| * | |||
| * As the selection is key-based, `ui5-table-row` components need to define a unique `key` property. | |||
| * As the selection is key-based, `ui5-table-row` components need to define a unique `row-key` property. | |||
There was a problem hiding this comment.
like F6 handling we should document the TableSelection import. better in the "features" slot documentation of the table as general suggestion and here in the TableSelection.ts
There was a problem hiding this comment.
it would be great to mention the import again in the documentation of the TableSelection
|
@aborjinik if you could review this PR, it would be great |
|
@ilhan007: @aborjinik is unavailable today (d-com), if we need to merge today, I'd need a +2 from you. |
|
|
||
| /** | ||
| * Interface for components that can be slotted inside the <code>features</code> slot of the <code>ui5-table</code>. | ||
| * | ||
| * @public | ||
| */ | ||
| interface ITableFeature extends UI5Element { | ||
| readonly identifier: string; |
There was a problem hiding this comment.
It would be nice to document what this is necessary for.
| @@ -80,6 +82,7 @@ type TableRowClickEventDetail = { | |||
| * | |||
| * The `ui5-table` can be enhanced in its functionalities by applying different features. | |||
| * Features can be slotted into the `features` slot, to enable them in the component. | |||
| * Features need to be imported separately, as they are not enabled by default. | |||
There was a problem hiding this comment.
It would be nice to give a sample here
e.g. in order to use the selection functionality, you need to import the following module:
import "@ui5/webcomponents/blabla"
| @@ -26,7 +26,7 @@ import { isSelectionCheckbox, isHeaderSelector, findRowInPath } from "./TableUti | |||
| * * Multiple - select multiple rows. | |||
| * * None - no selection active. | |||
| * | |||
| * As the selection is key-based, `ui5-table-row` components need to define a unique `key` property. | |||
| * As the selection is key-based, `ui5-table-row` components need to define a unique `row-key` property. | |||
There was a problem hiding this comment.
it would be great to mention the import again in the documentation of the TableSelection
Will do this tomorrow |
Table.ts imported TableSelection.ts, although it is not a base functionality.
Therefore, the selection has been removed as import and the code has been refactored to accomodate for that.
Additionaly, key is a reserved keyword in React, which is why the row key has been renamed to row-id.