-
Notifications
You must be signed in to change notification settings - Fork 356
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
fix: highlight Data Input
link
#9676
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
can we just use a Link
or something in OverviewStats
when it has an onClick
value? and remove style
attribute usage from here and "Resource Allocation"?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9676 +/- ##
==========================================
- Coverage 53.32% 48.70% -4.63%
==========================================
Files 1254 931 -323
Lines 152655 123420 -29235
Branches 3244 3242 -2
==========================================
- Hits 81398 60106 -21292
+ Misses 71105 63162 -7943
Partials 152 152
Flags with carried forward coverage won't be shown. Click here to find out more.
|
<span style={{ color: props.onClick ? 'var(--theme-status-active)' : 'inherit' }}> | ||
{props.children} | ||
</span> |
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.
does Link
not work?
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.
determined/webui/react/src/components/OverviewStats.tsx
Lines 17 to 32 in e956f28
const OverviewStats: React.FC<Props> = (props: Props) => { | |
const column = ( | |
<Column> | |
<Row> | |
<Label size={TypographySize.XS} truncate={{ tooltip: true }}> | |
{props.title} | |
</Label> | |
</Row> | |
<Row width="fill"> | |
<Label strong truncate={{ tooltip: true }}> | |
{props.children} | |
</Label> | |
</Row> | |
</Column> | |
); | |
return <Card>{props.onClick ? <Link onClick={props.onClick}>{column}</Link> : column}</Card>; |
it works in terms of the appearance, but that might cause nested anchor tag (<a>
) which is forbidden.
https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element
there must be no interactive content descendant
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.
honestly it's kind of weird we have a link for a specific line within a clickable card in the first place, but I'd prefer to use Link
, partially because it also has the hover styling.
I think the nested <a>
is only happening because the card content is also unnecessarily wrapped in a Link
when instead it should be using the Card
's onClick
(which also adds hover styling to the card itself)
suggestion:
return (
<Card onClick={props.onClick}>
<Column>
<Row>
<Label size={TypographySize.XS} truncate={{ tooltip: true }}>
{props.title}
</Label>
</Row>
<Row width="fill">
<Label strong truncate={{ tooltip: true }}>
{props.onClick ? (
<Link>
{props.children}
</Link>
) : props.children}
</Label>
</Row>
</Column>
</Card>
);
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.
fixed it. also unwraped <Link>
in card content
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 think we should probably still have onClick
on Card
?
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.
imo only blue text should be clickable. otherwise nested anchor issue would happen
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.
if product agrees, sure. my suggestion was to avoid removing functionality.
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.
product agreed with only text link (Slack)
Ticket
ET-638
Description
Blue color link instead of white link for
Data Input
card to let users know its clickableTest Plan
Data Input
card should have blue link in the experiment detail pageData Input
instead ofData input
Checklist
docs/release-notes/
See Release Note for details.