Showcase - Convert Table route to TS #3067
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| selectionKey, | ||
| selectionCheckboxElement, | ||
| }: HdsTableOnSelectionChangeSignature) { | ||
| if (selectionKey) { |
There was a problem hiding this comment.
[Note] selectionKey and selectionCheckboxElement are optional, so checks have been added to handle that use case. This applies to all similar selection change functions below.
| @tracked multiSelectToggleScope__demo1 = false; | ||
| @tracked multiSelectToggleDebug__demo1 = false; | ||
| @deepTracked multiSelectSelectableData__demo1 = [ | ||
| // @ts-expect-error - we need to reevaluate how we get the data for the table demos when we break up the template files into sub components |
There was a problem hiding this comment.
[Note] Without this, an error is flagged that the controller is using this.model before it is defined. Similar expect errors are added for other data variables.
| @@ -56,6 +56,7 @@ | |||
| "@glint/template": "^1.5.2", | |||
| "@hashicorp/design-system-components": "workspace:*", | |||
| "@hashicorp/design-system-tokens": "workspace:*", | |||
| "@nullvoxpopuli/ember-composable-helpers": "^5.2.11", | |||
There was a problem hiding this comment.
[Note] In the Table template the helper call is used, but its package, ember-composable-helpers, isn't included in the showcase devDependencies.
| console.log('Selected Row Keys:', selectedRowsKeys); | ||
| console.groupEnd(); | ||
| this.multiSelectUserData__demo4.forEach((user) => { | ||
| user.isSelected = selectedRowsKeys.includes(user.id); |
There was a problem hiding this comment.
[Note] There are type errors thrown here as user.id is a number, but selectionRowKeys expects a string. selectionRowKeys has been updated to support number in #3066 .
dchyun
force-pushed
the
dchyun/table-showcase-ts-conversion
branch
from
72e3176 to
bad62d5
Compare
| <B.Tr | ||
| @selectionKey={{B.data.id}} | ||
| {{! @glint-expect-error - will be fixed by https://hashicorp.atlassian.net/browse/HDS-5090}} | ||
| @selectionKey="{{B.data.id}}" |
There was a problem hiding this comment.
[Note] @selectionKey is typed as a string, but the data id is a number, so converting it to a string here, and in following similar instances.
| @@ -3,6 +3,7 @@ | |||
| SPDX-License-Identifier: MPL-2.0 | |||
| }} | |||
|
|
|||
| {{! @glint-expect-error - file will be broken up in https://hashicorp.atlassian.net/browse/HDS-5077}} | |||
There was a problem hiding this comment.
[Note] There is an error thrown about how lengthy this page is, this should be able to be removed once the page is broken up into sub templates.
dchyun
force-pushed
the
dchyun/table-showcase-ts-conversion
branch
from
bad62d5 to
9aa88c2
Compare
| @onClickSort={{fn H.setSortBy "peer-name"}} | ||
| @sortOrder={{if (eq "peer-name" H.sortBy) H.sortOrder}} | ||
| >Peer name</H.ThSort> | ||
| {{#if H.setSortBy}} |
There was a problem hiding this comment.
[Note] @didoo @shleewhite this is where the check for H.setSortBy is needed. In @onClickSort, setSortBy is called directly and because it is conditionally yielded it's possible it could be undefined.
There was a problem hiding this comment.
Left a few comments.
Can you open the equivalent files from the AdvancedTable side by side, and compare the implementation there and here, to make sure they're as similar as possible? It's not nitpicking: this consistency will facilitate future code refactoring, debugging, reviews, etc.
| s1 instanceof Object && | ||
| s2 instanceof Object && | ||
| 'status' in s1 && | ||
| 'status' in s2 && | ||
| typeof s1['status'] === 'string' && | ||
| typeof s2['status'] === 'string' |
There was a problem hiding this comment.
[question] at line 153 there is similar code (clonedModelClusters.sort) but it's not so "strictly typed" as here. Why in that case is not necessary, but here it is?
There was a problem hiding this comment.
The difference between this sorting function and others (like line 184 get sortedModelClusters__demo3()) is that this function is used in the sortingFunction column hash.
In the function I can type the parameters as MockDataCluster so s1['status'] is defined, but the problem is the parameters of the sortingFunction are typed as unknown so there is a type mismatch.
This implementation matches the example from the adv table so thinking to leave it as is for now.
dchyun
force-pushed
the
dchyun/table-showcase-ts-conversion
branch
from
9aa88c2 to
d640d31
Compare
@didoo Compared the adv table and table files and aligned in a few places to the adv table. Nothing major, but should be aligned for all similar functions now. |
|
@dchyun since #3074 is practically ready to be merged (only a couple of tiny things left to fix) do you think it would make sense to rebase you branch on that PR's branch, and use the data sources (and data types) from that PR? I think it could simplify this PR and improve even more the consistency between Table and AdvancedTable migrated code |
@didoo Yes that was my intention to wait for the data reorg to go in before rebasing and merging this. I agree it should simplify things a lot. |
dchyun
force-pushed
the
dchyun/table-showcase-ts-conversion
branch
from
9c0c88b to
d0ac2c1
Compare
| selectAllState: boolean, | ||
| ) => { | ||
| modelData.forEach((modelRow) => { | ||
| if (modelRow instanceof Object && 'isSelected' in modelRow) { | ||
| if (modelRow instanceof Object) { |
There was a problem hiding this comment.
[Note] The userData data doesn't have isSelected set, so this check was breaking the "delete row" example's "expand scope" toggle, which used the userData and this function. The type has been updated to be more specific to ensure modelRow.isSelected is available.
| @@ -42,7 +41,6 @@ export default class PageComponentsAdvancedTableRoute extends Route { | |||
| userDataDemo4: clone( | |||
| users.slice(0, 4).map((user) => ({ ...user, isAnimated: false })), | |||
| ), | |||
| manyColumns: userWithMoreColumns, | |||
There was a problem hiding this comment.
[Note] This data didn't appear to be being used anywhere in the adv table so I've removed it from the model
| @@ -10,6 +10,8 @@ import { deepTracked } from 'ember-deep-tracked'; | |||
| import { later } from '@ember/runloop'; | |||
|
|
|||
| import type { PageComponentsAdvancedTableModel } from 'showcase/routes/page-components/advanced-table'; | |||
| import type { SelectableItem } from 'showcase/mocks/selectable-item-data'; | |||
| @@ -42,7 +41,6 @@ export default class PageComponentsAdvancedTableRoute extends Route { | |||
| userDataDemo4: clone( | |||
| users.slice(0, 4).map((user) => ({ ...user, isAnimated: false })), | |||
| ), | |||
| manyColumns: userWithMoreColumns, | |||
There was a problem hiding this comment.
Thank you for catching this! Didn't realize its not used in the template.
📌 Summary
If merged, this PR would convert the pages in the showcase for the
Tablecomponent to TS.Showcase page
Note: Percy diff is expected as the
mediumdensityis now being displayed.🔗 External links
Jira ticket: HDS-5076
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.
📋 PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.