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

Popover: Base implementation using Floating-UI #3244

Merged
merged 7 commits into from Nov 2, 2023

Conversation

diyorbek
Copy link
Contributor

@diyorbek diyorbek commented Oct 11, 2023

Summary

Re-implemented core internal components of Popover using Floating-UI.
New implementation is not exposed. This is the first PR of gradual rehaul of Popover and Popover based components.

The changes include:

  • New Contents component
  • New Controller component
  • New InternalPopover
  • disablePortal prop for InternalPopover
  • scrollBoundary prop for InternalPopover
  • hideWhenReferenceHidden prop for InternalPopover (open for shorter prop name suggestions)

What changed?

Why?

Links

  • Jira
  • TDD
  • [Figma](link to Figma file)

Checklist

  • Added unit and Flow Tests
  • Added documentation + accessibility tests
  • Verified accessibility: keyboard & screen reader interaction
  • Checked dark mode, responsiveness, and right-to-left support
  • Checked stakeholder feedback (e.g. Gestalt designers)

@netlify
Copy link

netlify bot commented Oct 11, 2023

Deploy Preview for gestalt ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 20b6020
🔍 Latest deploy log https://app.netlify.com/sites/gestalt/deploys/6543ae82e1dda000082ca9e7
😎 Deploy Preview https://deploy-preview-3244--gestalt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@diyorbek diyorbek changed the title Popover: New implementation using Floating-UI Popover: Base implementation using Floating-UI Oct 31, 2023
@diyorbek diyorbek marked this pull request as ready for review October 31, 2023 18:53
@diyorbek diyorbek requested a review from a team as a code owner October 31, 2023 18:53
@diyorbek diyorbek added the patch release Patch release label Oct 31, 2023
'top': 'up',
};

interface Props {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a description of each prop here?

/**
  * description
  */

scrollBoundary?: HTMLElement,
};

export default function Contents({
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change name here or in File to NewContents.

I know locations / paths are different but I want to make sure we dont exhange them by misatke. we cant rerename in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about Experimental prefix? Or we can rename old ones with prefix Legacy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll leave it up to you. Legacy maybe makes more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed old one with Legacy prefix

@diyorbek diyorbek merged commit a533f2c into pinterest:master Nov 2, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch release Patch release
Projects
None yet
2 participants