-
Notifications
You must be signed in to change notification settings - Fork 43
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
🌱 Add data attributes to table rows #1996
Conversation
808260f
to
62be899
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1996 +/- ##
==========================================
+ Coverage 39.20% 42.21% +3.01%
==========================================
Files 146 171 +25
Lines 4857 5477 +620
Branches 1164 1352 +188
==========================================
+ Hits 1904 2312 +408
- Misses 2939 3049 +110
- Partials 14 116 +102
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
62be899
to
814abac
Compare
814abac
to
8631a82
Compare
@@ -83,7 +83,8 @@ export const BusinessServices: React.FC = () => { | |||
|
|||
const tableControls = useLocalTableControls({ | |||
tableName: "business-services-table", | |||
idProperty: "name", | |||
idProperty: "id", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch! please mention those fixes in the commit message (or extract to a small PR)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch! please mention those fixes in the commit message (or extract to a small PR)?
I mentioned that kind of change in the description as "and if necessary, adjust the idProperty". Could be called out more explicitly.
Also interesting that it really didn't do anything bad or wrong with any behavior. Apparently the name on the entity is enforced unique so it worked just fine. But now both bits of info are available on the data attributes. :-)
client/src/app/pages/applications/manage-imports-details/manage-imports-details.tsx
Show resolved
Hide resolved
client/src/app/pages/applications/manage-imports-details/manage-imports-details.tsx
Show resolved
Hide resolved
client/src/app/pages/applications/manage-imports-details/manage-imports-details.tsx
Show resolved
Hide resolved
@@ -74,6 +74,7 @@ export const ManageImportsDetails: React.FC = () => { | |||
const tableControls = useLocalTableControls({ | |||
tableName: "manage-imports-details", | |||
idProperty: "Application Name", | |||
dataNameProperty: "Application Name", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am probably missing something...
Shouldn't it be one of the columnNames
value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the original idProperty
name didn't make much sense to me either. Tracing the code back, and the data is coming from:
tackle2-ui/client/src/app/api/rest.ts
Lines 315 to 321 in caac127
export const getApplicationImports = ( | |
importSummaryID: number, | |
isValid: boolean | string | |
): Promise<ApplicationImport[]> => | |
axios | |
.get(`${APP_IMPORT}?importSummary.id=${importSummaryID}&isValid=${isValid}`) | |
.then((response) => response.data); |
And the return type is:
tackle2-ui/client/src/app/api/models.ts
Lines 180 to 184 in caac127
export interface ApplicationImport { | |
"Application Name": string; | |
errorMessage: string; | |
isValid: boolean; | |
} |
I haven't looked at the hub code to see if other data items are also returned with that endpoint. Working with what we already have, the only field that makes sense in the ApplicationImport
to use for the idProperty
and dataNameProperty
is "Application Name"
.
I wouldn't touch this any further in the PR, but we can write an issue to re-evaluate it in the near future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, while the columnNames
for the table properties often line up with the actual item field name, there is no requirement that it do so. In some tables, the columns are "UI synthetic" being derived from multiple data sources.
That's just a long way of saying the idProperty
and dataNameProperty
values do not need to have a corresponding entry in columnNames
.
8631a82
to
f5b3458
Compare
To enable easier and safer selection of table rows by testing code, add some data attributes to the tr: - `data-item-id` - Holds the `idProperty` value of a table's items. This is typically the item's id number but may be a frontend generated ID in some cases. - `data-item-name` - If the table property `dataNameProperty` is provided, hold the name property of a table's item. Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[email protected]>
f5b3458
to
dbb3509
Compare
To enable easier and safer selection of table rows by testing code, add some data attributes to the tr:
data-item-id
- Holds theidProperty
value of a table's items. This is typically the item's id number but may be a frontend generated ID in some cases.data-item-name
- If the table propertydataNameProperty
is provided, hold the name property of a table's item.For any tables that use
/use.?*TableControls\(/
, add adataNameProperty
and if necessary, adjust theidProperty
. A few tables for items without a good item id and name property were left with just theidProperty
.