chore: Underline styles for popover text trigger #3060
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3060 +/- ##
========================================
Coverage 96.42% 96.42%
========================================
Files 787 787
Lines 22191 22191
Branches 7555 7209 -346
========================================
Hits 21398 21398
- Misses 741 786 +45
+ Partials 52 7 -45 ☔ View full report in Codecov by Sentry. |
georgylobko
requested review from
michaeldowseza
and removed request for
a team
|
Are all visual differences to be expected from this? e.g. Slight change in dotted line + slight shift in where the popover appears from?
|
There was a problem hiding this comment.
@michaeldowseza
I’ve addressed this discrepancy and reviewed the screenshot tests in my dev pipeline. Everything looks good now.
| @@ -12,6 +12,14 @@ | |||
| @import './container'; | |||
| @import './motion'; | |||
|
|
|||
| $trigger-underline-offset: 0.25em; | |||
There was a problem hiding this comment.
0.25 is used instead of 0.2 because an underline at 0.2 overlaps with the text
…n the trigger and the bottom-positioned popover
georgylobko
requested review from
gethinwebster and
pan-kot
and removed request for
gethinwebster
src/popover/styles.scss
Outdated
| } | ||
|
|
||
| /* stylelint-disable selector-combinator-disallowed-list, selector-max-type, selector-max-universal */ | ||
| & span { |
There was a problem hiding this comment.
Would it make sense to add a span for both in-place and portal variants of the popover to avoid the condition in styles?
I also see no reason to violate the stylelint rules: you can add a class name to the span element and target it directly.
There was a problem hiding this comment.
Unfortunately, that is not a case, because the styles are applied when we are dealing with 'external' component which we have no control over. See the example below:
<Popover
size="medium"
position="right"
header="Memory error"
content={<DynamicContent />}
dismissAriaLabel="Close"
renderWithPortal={renderWithPortal}
>
<StatusIndicator type="error">Error</StatusIndicator>
</Popover>The current implementation applies border styles directly to the popover trigger, which is a block element, while underline styles are not inherited down the block elements, only for inline elements. Therefore, I realized that the only way to apply the styles to the block element is by using this selector.
There was a problem hiding this comment.
What if the nested element is not a span?
There was a problem hiding this comment.
Since this change specifically applies to text type of triggers, I think it's a reasonable assumption that only span elements will be used. This code specifically targets our components and use cases. If something doesn't work well for users, they always have the option to introduce a custom trigger
src/popover/styles.scss
Outdated
| line-height: calc(1 + #{$trigger-underline-offset} + #{awsui.$border-divider-list-width}); | ||
| } | ||
|
|
||
| /* stylelint-disable selector-combinator-disallowed-list, selector-max-type, selector-max-universal */ |
There was a problem hiding this comment.
I would add a code comment explaining that the combinator actually targets the custom content
Description
AWSUI-59855
Replaced border bottom with underline dotted styles.
Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.