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

Update PositionedOverlay to support HoverCard positioning #11758

Closed
wants to merge 6 commits into from

Conversation

yurm04
Copy link
Contributor

@yurm04 yurm04 commented Mar 18, 2024

WHY are these changes introduced?

Pulls in the updates to PositionedOverlay that were originally going to go out as part of the AlphaHoverCard PR

Fixes #11748

WHAT is this pull request doing?

Adds left and right positioning support for overlays

How to 🎩

Check the local storybook and/or visit this spin and hover over the Destination cell to the the right positioned HoverCard

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

@yurm04
Copy link
Contributor Author

yurm04 commented Mar 18, 2024

/snapit

1 similar comment
@yurm04
Copy link
Contributor Author

yurm04 commented Mar 18, 2024

/snapit

Copy link
Contributor

🫰✨ Thanks @yurm04! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/[email protected]
yarn add @shopify/[email protected]
yarn add @shopify/[email protected]

Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

Thanks @yurm04! Changes brought over all look correct, except where noted that one of Kyle's recent changes was accidentally reverted. Do you have a Spinstance we can tophat? We also need to bring over the Tooltip change to the preferredPosition prop that only allows it to be above or below.

.changeset/real-snails-turn.md Outdated Show resolved Hide resolved
@@ -270,10 +289,9 @@ export class PositionedOverlay extends PureComponent<
: this.firstScrollableContainer;
const scrollableContainerRect = getRectForNode(scrollableElement);

const overlayRect =
fullWidth || preferredPosition === 'cover'
Copy link
Member

Choose a reason for hiding this comment

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

This we need to bring in--this was part of Kyle's recent addition of cover position.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch! ty @chloerice 🙏

@kyledurand adding you as a reviewer to make sure I didn't bulldoze any of your recent changes

fullWidth || preferredPosition === 'cover'
? new Rect({...currentOverlayRect, width: activatorRect.width})
: currentOverlayRect;
const overlayRect = fullWidth
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
const overlayRect = fullWidth
fullWidth || preferredPosition === 'cover'

@yurm04 yurm04 marked this pull request as ready for review March 19, 2024 16:27
@yurm04
Copy link
Contributor Author

yurm04 commented Mar 19, 2024

/snapit

Copy link
Contributor

🫰✨ Thanks @yurm04! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/[email protected]
yarn add @shopify/[email protected]
yarn add @shopify/[email protected]

Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Looks like this removes cover positioning. Might just be a bad rebase but not working when I tophat

@@ -17,12 +17,15 @@ import {
import type {PreferredPosition, PreferredAlignment} from './utilities/math';
import styles from './PositionedOverlay.module.css';

type Positioning = 'above' | 'below' | 'cover';
type Positioning = 'above' | 'below' | 'left' | 'right' | 'cover';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be start end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't a hard opinion, but I think left and right make sense here because it is relative to the activator 🤔 but I'm not married to that and happy to change it to be consistent with the rest of our positioning props

Copy link
Contributor

Choose a reason for hiding this comment

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

It matches with the actual css positioning as well so I'm fine with it

Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

Commented the missing logic to bring over for preferredPosition="cover" (locally tested diff of suggested changes for reference)


const chevronOffset =
activatorRect.center.x -
horizontalPosition +
overlayMargins.horizontal * 2;

let width = null;

if (fullWidth) width = overlayRect.width;
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
if (fullWidth) width = overlayRect.width;
if (fullWidth || preferredPosition === 'cover') width = overlayRect.width;

: spaceAbove - minimumSurroundingSpace;
const heightIfBelow = enoughSpaceFromBottomEdge
? desiredHeight - verticalMargins
: spaceBelow - minimumSurroundingSpace;
Copy link
Member

@chloerice chloerice Mar 25, 2024

Choose a reason for hiding this comment

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

Suggested change
: spaceBelow - minimumSurroundingSpace;
: spaceBelow - minimumSurroundingSpace;
const heightIfAboveCover = Math.min(
spaceAbove + activatorRect.height,
desiredHeight,
);
const heightIfBelowCover = Math.min(
spaceBelow + activatorRect.height,
desiredHeight,
);
const positionIfCoverBelow = {
height: heightIfBelowCover - verticalMargins,
top: activatorTop + containerRectTop,
positioning: 'cover',
};
const positionIfCoverAbove = {
height: heightIfAboveCover - verticalMargins,
top:
activatorTop +
containerRectTop -
heightIfAbove +
activatorRect.height +
verticalMargins,
positioning: 'cover',
};
if (preferredPosition === 'cover') {
return (enoughSpaceFromBottomEdge ||
(mostSpaceOnBottom &&
spaceBelow + activatorRect.height > desiredHeight) ||
spaceBelow > spaceAbove)
? positionIfCoverBelow
: positionIfCoverAbove;
}

@@ -20,133 +28,210 @@ export function calculateVerticalPosition(
fixed: boolean | undefined,
topBarOffset = 0,
) {
Copy link
Member

@chloerice chloerice Mar 25, 2024

Choose a reason for hiding this comment

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

Make TS happy in PositionedOverlay.tsx

Suggested change
) {
: {
top?: number;
bottom?: number;
height: number;
positioning: string;
}

@chloerice
Copy link
Member

chloerice commented Apr 2, 2024

@yurm04, please make sure to prevent Tooltip from supporting left and right, as that is not UX that we want.

TooltipOverlay.jsx

preferredPosition?: 'above' | 'below';

@chloerice chloerice force-pushed the positioned-overlay-hovercard branch from f9c8d99 to fa4d0a7 Compare April 17, 2024 20:46
@chloerice
Copy link
Member

/snapit

Copy link
Contributor

🫰✨ Thanks @chloerice! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240417204842"

@github-actions github-actions bot added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 19, 2024
@translation-platform
Copy link
Contributor

Localization quality issues found

The following issues may affect the quality of localized translations if they are not addressed:

  • The value Clear for key Polaris.ActionList.SearchField.clearButtonLabel is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
  • The value Search for key Polaris.ActionList.SearchField.search is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
  • The value Search actions for key Polaris.ActionList.SearchField.placeholder is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.

Please look out for other instances of this issue in your PR and fix them as well if possible.

Questions about these messages? Hop in the #help-localization Slack channel.

Copy link
Contributor

Hi! We noticed there hasn’t been activity on this PR in a while. After 30 days, it will close automatically.

If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.

@chloerice chloerice closed this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. no-pr-activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update PositionedOverlay with original AlphaHovercard changes and release independently
3 participants