gabotorresruiz commented on PR #43584:
URL: https://github.com/apache/superset/pull/43584#issuecomment-5589948727
Hi @FrancescoCastaldi, thanks for the quick EOF fix, and nice work on the
conflict resolution in general: you kept everything #43425 added (the aria
attributes, the hover and focus suppression wiring, the tooltip delay changes)
and layered your structural change on top. I verified this branch in a running
instance: moving from the Date Range pill up to the info icon shows only the
description tooltip, hovering the pill still shows the range tooltip, and
keyboard-focusing the icon opens the description. Behavior is identical to
current master in every scenario I tested, and CI is green.
One last thing before I can approve: could you update the title and
description to match what the PR now does? #43386 is already fixed and closed
by #43425, so this no longer fixes that issue, and the root cause described
(form label association) is not what was happening: `htmlFor={name}` points at
nothing, since no element has `id="time_range"`. I would frame this as a
cleanup that moves the optional icons out of the `<label>` for HTML validity,
following up on #43425. The description also still mentions a change to the
`packages/superset-ui-chart-controls` copy of `ControlHeader` that is not in
the diff.
Just a small NIT, fine as a follow-up: now that the icon lives outside the
label, the `role="button"` span workaround (and the comment explaining it) is
no longer forced on us, so it could become a real `<button>`.
Once the description is updated this is good to go. Thanks for sticking with
it!
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
