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(Popover): adjust popover content placement #10539

Merged

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented Jan 27, 2022

Closes #10286

Minor adjustment to the placement of the popover content which should line up the caret (pointy thing) with the end of the text padding
145869848-c89e4798-d3df-4036-9701-8021dbfa6f85

Changelog

Testing / Reviewing

make sure whats compiled and on netlify matches the above screen shot 👍🏾

@dakahn dakahn requested a review from a team as a code owner January 27, 2022 04:03
@dakahn dakahn requested review from joshblack and abbeyhrt January 27, 2022 04:03
@netlify
Copy link

netlify bot commented Jan 27, 2022

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 9ffc2f0

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/620d3eec0a182c00085cef00

😎 Browse the preview: https://deploy-preview-10539--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Jan 27, 2022

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 9ffc2f0

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/620d3eec85a0da0007d9a0ac

😎 Browse the preview: https://deploy-preview-10539--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Jan 27, 2022

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 9ffc2f0

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/620d3eec081fc200079eb5c9

😎 Browse the preview: https://deploy-preview-10539--carbon-components-react.netlify.app

@dakahn dakahn requested a review from laurenmrice January 27, 2022 15:57
@joshblack
Copy link
Contributor

@laurenmrice @dakahn this might be coming up due to some of the changes we made to the placement logic and I wanted to see what you all felt like was the right set of trade-offs.

When looking at the popover, we have two pieces: the caret and the overall box

image

When we are placing these two pieces, everything is relative to the trigger. As a result, we can only place these items in the center of the trigger, or left/right-aligned

image

Screen Shot 2022-01-27 at 11 00 31

Screen Shot 2022-01-27 at 11 00 40

Unfortunately, we can't really control how much distance there is between the edge of the content and the position of the caret.

If we want to control the distance between the edge of the content and the position of the caret, then unfortunately we lose the ability to center the caret relative to the trigger 😞 This is a trade-off and I'm curious what you all think / which one we want to pick.

I'll also share this with the team to see if they have any other thoughts on how to tackle this!

@joshblack
Copy link
Contributor

Notes from our meeting today:

image

It seems like we need to:

  • Revert the changes to the default placement so that the edge of the popover aligns to the edge of the trigger
  • Add in a caret only translation that follows the formula:
    • Center popover
    • Translate half the height (or width) of the caret
    • Add the popover padding value

@abbeyhrt
Copy link
Contributor

abbeyhrt commented Feb 7, 2022

@dakahn Is this ready for review? I wasn't sure based on the comments above 🙂

@laurenmrice laurenmrice mentioned this pull request Feb 7, 2022
29 tasks
@laurenmrice laurenmrice mentioned this pull request Feb 8, 2022
29 tasks
@dakahn dakahn requested a review from tay1orjones February 8, 2022 23:25
@dakahn
Copy link
Contributor Author

dakahn commented Feb 8, 2022

@abbeyhrt should be good for a review now 👍🏾

@laurenmrice
Copy link
Member

For some reason the build/preview keeps failing, once it's fixed I can review 🙂.

@dakahn
Copy link
Contributor Author

dakahn commented Feb 10, 2022

@laurenmrice should be good to go at this link. If that's still weird shift-click the reload button -- might be a caching problem

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

The alignments left-bottom, left-top, right-bottom, and right-top are having some misalignment issues when the caret prop is set to false.
popover


It should look like this spec:
Artboard


All the other alignments look great!

@dakahn
Copy link
Contributor Author

dakahn commented Feb 10, 2022

@laurenmrice ACK! Great catch. Totally missed that. On it 👍🏾

@dakahn dakahn requested a review from laurenmrice February 11, 2022 01:11
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Looks good to me now D.A.! Thank you! ✅

@kodiakhq kodiakhq bot merged commit 9bc8ad8 into carbon-design-system:main Feb 16, 2022
@jnm2377 jnm2377 mentioned this pull request Feb 22, 2022
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[popover]: caret should be 16px away from edge of popover
5 participants