rusackas commented on a change in pull request #11134:
URL:
https://github.com/apache/incubator-superset/pull/11134#discussion_r500761039
##########
File path: superset-frontend/src/components/ListView/TableCollection.tsx
##########
@@ -260,6 +264,7 @@ export default function TableCollection({
prepareRow(row);
return (
<tr
+ data-test="table-row"
Review comment:
I am a bit nervous about cases like this where we're adding a bunch of
HTML markup for ever row and cell. If we strip these data-test attributes out
for production builds, then I'm not worried about it.
In any case, I _love_ the changes in this PR where the data-test attribute
points at a _specific instance_ of something, making the test more
traceable/fixable, but I'm wondering if examples like this are adding much
value, since the data-test attribute is just a twin of the class. I guess it
doesn't hurt, and separates the concerns, so the class (for styling) can be
safely changed/renamed/removed/conditional.
I guess it's fine, just a little concerned about the value-to-markup ratio.
If we strip out the markup for prod, it's not a problem to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
