-
Notifications
You must be signed in to change notification settings - Fork 34
DONT-REVIEW-TEST #3039
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
base: alpha
Are you sure you want to change the base?
DONT-REVIEW-TEST #3039
Conversation
23cb3de
to
95ff993
Compare
069e025
to
0ab5690
Compare
-Added React playground code, and rebased with alpha |
width: width.includes("%") ? width : `min(${width}, 100%)`, | ||
}); | ||
const finalWidth = width.includes("%") ? width : `min(${width}, 100%)`; | ||
const cssProps: Record<string, string> = { width: finalWidth }; |
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 there's a slight issue. width
and max-width
and min-width
are all being applied simultaneously in different spots. You're applying width
, and max-width
here in this onMount
.
width
is also applied on line 172 inside the divroot
max-width
is also applied on line 230 inside the CSS for.root
min-width
andmax-width
are also being applied on lines 283 and 284 inside the CSS fortextarea
min-width
andmax-width
are also being applied on lines 329, 330, and 336, 337 inside the CSS for mobile and for not-mobile
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.
Might be because of above, I'm not sure. But TextArea width
set to a percentage value doesn't work as expected.
// Public | ||
export let content = ""; | ||
export let testid: string = ""; |
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.
Tooltip using maxWidth
with maxWidth
set to a percentage doesn't display accurately
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.
Using ch
also doesn't work as expected:
maxWidth
set to 10ch:
Using this code:
<goab-block direction="column" gap="m">
<goab-text tag="h2">Tooltip examples</goab-text>
<goab-grid minChildWidth="260px" gap="m">
<goab-block
*ngFor="let example of tooltipExamples; index as index"
direction="column"
gap="s"
>
<goab-text tag="h3">{{ example.label }}</goab-text>
<goab-text tag="p">{{ example.description }}</goab-text>
<goab-tooltip
[content]="example.content"
[maxWidth]="example.maxWidth"
[style.width]="example.width"
>
<goab-icon type="information-circle" size="3"></goab-icon>
</goab-tooltip>
</goab-block>
</goab-grid>
</goab-block>
readonly tooltipExamples: TooltipExample[] = [
{
label: "Default tooltip",
description: "Relies on the default tooltip sizing for comparison.",
content:
"This tooltip keeps the default sizing so longer text can expand without additional constraints.",
},
{
label: "Style width 500px, maxWidth 200px",
description: "Applies an inline width while capping the tooltip content at 200px.",
width: "500px",
maxWidth: "200px",
content:
"Inline width is set to 500px but the tooltip maxWidth is restricted to 200px to demonstrate clamping.",
},
{
label: "Max width 300px",
description: "Limits the tooltip content to 300px without changing width.",
maxWidth: "100ch",
content:
"The tooltip content should wrap within 300px, demonstrating the maxWidth attribute in pixels.",
},
{
label: "Max width 50%",
description: "Uses a responsive maxWidth to scale with the parent container.",
width: "500px",
maxWidth: "100%",
content:
"This tooltip uses a percentage based maxWidth so it will shrink or grow with its container width.",
},
];
export let value: string | undefined = ""; | ||
export let filterable: string = "false"; | ||
export let leadingicon: GoAIconType | null = null; | ||
export let maxheight: string = "276px"; |
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.
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.
PR updated and angular playground code updated also.
describe("Dropdown Component", () => { | ||
const noop = () => { | ||
// noop | ||
}; |
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.
For all the browser tests, you're only testing basic px
measurements. You should also have tests using %
and ch
to make sure those work as well.
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.
PR updated.
const spaceLeft = rootRect.left; | ||
const spaceRight = window.innerWidth - rootRect.right; | ||
const calculatedMaxWidth = maxwidth ? parseFloat(maxwidth) : 400; |
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.
This won't work in the case that a user specifies a non-px value. Since the calculation is required to determine which side the tooltip will appear on, for now let's restrict them to using px values.
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.
PR updated.
export let rows: number = 3; | ||
export let testid: string = ""; | ||
export let width: string = "60ch"; | ||
export let maxwidth: string = ""; |
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.
To prevent the current width
default property from conflicting with the maxwidth
, change to the following
export let width: string = "100%"
export let maxwidth: string = "60ch"
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.
Currently when setting the maxWidth it behaves like width, in that if the screen is resized the component stays set at the max width (this is true for both the textarea and dropdown), but it would be assumed by devs that the component would either be at 100%
or the maxwidth.
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.
PR updated.
5a9a809
to
c2d2b54
Compare
Before (the change)
After (the change)
Make sure that you've checked the boxes below before you submit the PR
Steps needed to test
Playground Code
Angular
angular-issue-2054.html.txt
angular-issue-2054.ts.txt
React