Skip to content
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.

Sign up for GitHub

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

chore: Underline styles for popover text trigger #3060

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

chore: Underline styles for popover text trigger #3060

Changes from 10 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7c3d841
chore: Underline styles for popover text trigger
georgylobko Nov 26, 2024
b2c3f2d
Merge branch 'main' into chore/popover-text-trigger-styles
georgylobko Dec 2, 2024
11f3222
chore: Styles adjustment
georgylobko Dec 4, 2024
cb6a3d3
chore: Add transparent border to keep text trigger's size
georgylobko Dec 5, 2024
587d50e
Merge branch 'main' into chore/popover-text-trigger-styles
georgylobko Dec 5, 2024
f1e42b9
chore: Apply text decoration for child span
georgylobko Dec 5, 2024
45d0a16
chore: Handle wrapTriggerText differently
georgylobko Dec 10, 2024
639d40f
Merge branch 'main' into chore/popover-text-trigger-styles
georgylobko Jan 14, 2025
a9143a1
Merge branch 'main' into chore/popover-text-trigger-styles
georgylobko Jan 15, 2025
178c7ce
feat: Introduce unified styles for both text wrapped and unwrapped po…
georgylobko Jan 15, 2025
f3476b8
chore: Introduce a transparent border-bottom to maintain space betwee…
georgylobko Jan 15, 2025
4109139
Merge branch 'main' into chore/popover-text-trigger-styles
georgylobko Jan 16, 2025
7fc1f38
Merge branch 'main' into chore/popover-text-trigger-styles
georgylobko Jan 16, 2025
5b8d48b
Merge branch 'main' into chore/popover-text-trigger-styles
georgylobko Jan 16, 2025
ac0d820
Merge branch 'main' into chore/popover-text-trigger-styles
georgylobko Jan 20, 2025
05282ff
chore: Change styles specificity
georgylobko Jan 20, 2025
9ee34f4
chore: Add a comment to explain the css rule to propagate down underl…
georgylobko Jan 21, 2025
6c46557
Merge branch 'main' into chore/popover-text-trigger-styles
georgylobko Jan 21, 2025
c329d84
feat: New triggerType in Popover component
georgylobko Jan 22, 2025
3cefaa0
Merge branch 'main' into chore/popover-text-trigger-styles
georgylobko Jan 22, 2025
41ba7a4
chore: Styles refactoring
georgylobko Jan 22, 2025
c5fc150
chore: Update api description
georgylobko Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion src/popover/styles.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@
@import './container';
@import './motion';

$trigger-underline-offset: 0.25em;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.25 is used instead of 0.2 because an underline at 0.2 overlaps with the text


@mixin trigger-underline-styles {
text-decoration: underline dashed currentColor;
text-decoration-thickness: awsui.$border-divider-list-width;
text-underline-offset: $trigger-underline-offset;
}

.root {
@include styles.styles-reset;
color: inherit;
Expand Down Expand Up @@ -46,7 +54,21 @@
background-color: transparent;

cursor: pointer;
border-block-end: awsui.$border-divider-list-width dashed currentColor;

@include trigger-underline-styles;

&.overflow-ellipsis {
/*
this line-height must be overridden, because of the overflow: hidden,
otherwise the underline styles would be hidden
*/
line-height: calc(1 + #{$trigger-underline-offset} + #{awsui.$border-divider-list-width});
}

/* stylelint-disable selector-combinator-disallowed-list, selector-max-type, selector-max-universal */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a code comment explaining that the combinator actually targets the custom content

& span {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@georgylobko georgylobko Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the nested element is not a span?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@include trigger-underline-styles;
}

&:focus {
outline: none;
Expand Down
Loading