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

fix: arrow breaking in AILabel popover #17982

Merged
merged 19 commits into from
Dec 17, 2024
Merged

fix: arrow breaking in AILabel popover #17982

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
1548b8f
fix: arrow breaking in AILabel popover
preetibansalui Nov 6, 2024
6268c8a
fix: removed console
preetibansalui Nov 6, 2024
1049e4b
fix: format issue
preetibansalui Nov 6, 2024
e22d97b
Merge branch 'main' into 17965-arrow-breaking-in-AILabel-popover
preetibansalui Nov 6, 2024
00a1a41
fix: format issue
preetibansalui Nov 7, 2024
1fe7edd
Merge branch 'main' into 17965-arrow-breaking-in-AILabel-popover
preetibansalui Nov 7, 2024
37c1700
fix: format issue
preetibansalui Nov 7, 2024
448b5ba
fix: format issue
preetibansalui Nov 7, 2024
511cf4a
Merge branch 'main' into 17965-arrow-breaking-in-AILabel-popover
preetibansalui Nov 13, 2024
105eb43
fix: arrow position vertically in case of ai=label
preetibansalui Nov 13, 2024
422144a
fix: added test
preetibansalui Nov 13, 2024
bb5885c
fix: format issue
preetibansalui Nov 13, 2024
3a0b131
fix: format
preetibansalui Nov 13, 2024
2586a46
chore: yarn format
preetibansalui Nov 13, 2024
e7ca76b
Merge branch 'main' into 17965-arrow-breaking-in-AILabel-popover
preetibansalui Nov 14, 2024
b83e44d
Merge branch 'main' into 17965-arrow-breaking-in-AILabel-popover
preetibansalui Nov 18, 2024
a74c86a
Merge branch 'main' into 17965-arrow-breaking-in-AILabel-popover
preetibansalui Nov 20, 2024
2d55a76
Merge branch 'main' into 17965-arrow-breaking-in-AILabel-popover
preetibansalui Nov 25, 2024
fa64469
Merge branch 'main' into 17965-arrow-breaking-in-AILabel-popover
preetibansalui Nov 27, 2024
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
8 changes: 7 additions & 1 deletion packages/react/src/components/AILabel/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ export const AILabel = React.forwardRef<HTMLDivElement, AILabelProps>(
? `${aiText} - ${slugLabel || ariaLabel}`
: `${aiText} - ${aiTextLabel || textLabel}`;

const isSmallIcon = ['xs', '2xs', 'mini'].includes(size);

return (
<div className={aiLabelClasses} ref={ref} id={id}>
{revertActive ? (
Expand All @@ -205,7 +207,11 @@ export const AILabel = React.forwardRef<HTMLDivElement, AILabelProps>(
<Undo />
</IconButton>
) : (
<Toggletip align={align} autoAlign={autoAlign} {...rest}>
<Toggletip
align={align}
autoAlign={autoAlign}
alignmentAxisOffset={isSmallIcon ? -24 : 0}
{...rest}>
<ToggletipButton
className={aiLabelButtonClasses}
label={ariaLabelText}>
Expand Down
11 changes: 9 additions & 2 deletions packages/react/src/components/Popover/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ export const Popover: PopoverComponent = React.forwardRef(
highContrast = false,
onRequestClose,
open,
alignmentAxisOffset,
preetibansalui marked this conversation as resolved.
Show resolved Hide resolved
...rest
}: PopoverProps<E>,
forwardRef: ForwardedRef<Element>
Expand Down Expand Up @@ -243,7 +244,6 @@ export const Popover: PopoverComponent = React.forwardRef(
}
}
});

const { refs, floatingStyles, placement, middlewareData } = useFloating(
enableFloatingStyles
? {
Expand All @@ -257,7 +257,14 @@ export const Popover: PopoverComponent = React.forwardRef(

// Middleware order matters, arrow should be last
middleware: [
offset(!isTabTip ? popoverDimensions?.current?.offset : 0),
offset(
!isTabTip
? {
alignmentAxis: alignmentAxisOffset,
mainAxis: popoverDimensions?.current?.offset,
Comment on lines +266 to +267
Copy link
Member

Choose a reason for hiding this comment

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

This isn't something blocking this PR and doesn't need changed, but one concern I just mentioned a comment on #17003 (comment) is that as we add more of these config values, they bloat the interface/api of these components. Especially when we haven't identified a use case from a consumer for this, it's just something we need internally for our use w/ AILabel.

The mainAxis offset is already pulled from a custom property instead of a prop value. Doing this for other values as well might be one way to scale these types of config values without having to have a prop for each one. Could be used in tandem w/ however we end up providing a way for consumers to config floating-ui from anywhere in their react tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understood your point, I added a prop here instead of some calculations so that it could be changed to different cases as well which might come in future as we have now for AILabel.

}
: 0
),
autoAlign &&
flip({
fallbackPlacements: align.includes('bottom')
Expand Down
30 changes: 18 additions & 12 deletions packages/react/src/components/Toggletip/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ import { usePrefix } from '../../internal/usePrefix';
import { PolymorphicProps } from '../../types/common';

type ToggletipLabelProps<E extends ElementType> = {
as?: E | undefined;
as?: E;
children?: ReactNode;
className?: string | undefined;
className?: string;
};

/**
Expand Down Expand Up @@ -82,12 +82,13 @@ function useToggletip() {
}

interface ToggletipProps<E extends ElementType> {
align?: PopoverAlignment | undefined;
as?: E | undefined;
autoAlign?: boolean | undefined;
className?: string | undefined;
align?: PopoverAlignment;
alignmentAxisOffset?: number;
as?: E;
autoAlign?: boolean;
className?: string;
children?: ReactNode;
defaultOpen?: boolean | undefined;
defaultOpen?: boolean;
}

/**
Expand Down Expand Up @@ -223,6 +224,11 @@ Toggletip.propTypes = {
'right-start',
]),

/**
* Provide an offset value for alignment axis.
*/
alignmentAxisOffset: PropTypes.number,

/**
* Provide a custom element or component to render the top-level node for the
* component.
Expand Down Expand Up @@ -253,8 +259,8 @@ Toggletip.propTypes = {

interface ToggletipButtonBaseProps {
children?: ReactNode;
className?: string | undefined;
label?: string | undefined;
className?: string;
label?: string;
}

export type ToggleTipButtonProps<T extends React.ElementType> =
Expand All @@ -265,7 +271,7 @@ export type ToggleTipButtonProps<T extends React.ElementType> =
* clicks and keyboard interactions.
*/
export const ToggletipButton = React.forwardRef(function ToggletipButton<
T extends React.ElementType,
T extends React.ElementType
>(
{
children,
Expand Down Expand Up @@ -323,7 +329,7 @@ ToggletipButton.displayName = 'ToggletipButton';

interface ToggletipContentProps {
children?: ReactNode;
className?: string | undefined;
className?: string;
}

/**
Expand Down Expand Up @@ -365,7 +371,7 @@ export { ToggletipContent };

interface ToggleTipActionsProps {
children?: ReactNode;
className?: string | undefined;
className?: string;
}

/**
Expand Down
Loading