Skip to content

refactor(popover): update styles and add new story#9965

Merged
kodiakhq[bot] merged 15 commits into
carbon-design-system:mainfrom
joshblack:feat/update-popover
Nov 3, 2021
Merged

refactor(popover): update styles and add new story#9965
kodiakhq[bot] merged 15 commits into
carbon-design-system:mainfrom
joshblack:feat/update-popover

Conversation

@joshblack

@joshblack joshblack commented Oct 27, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Reference #9941

Changelog

New

  • Add tests for popover

Changed

  • Update popover to use new tokens and update styles to use new CSS features now available

Removed

Testing / Reviewing

  • Look at the story in storybook and verify that it works in every direction

@joshblack joshblack requested a review from a team as a code owner October 27, 2021 16:19
@netlify

netlify Bot commented Oct 27, 2021 •
edited
Loading

Copy link
Copy Markdown

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

🔨 Explore the source changes: 91b4c65

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

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

@joshblack joshblack requested a review from laurenmrice October 27, 2021 16:19
abbeyhrt
abbeyhrt reviewed Oct 27, 2021
View reviewed changes
Comment thread packages/styles/scss/components/popover/_popover.scss
@netlify

netlify Bot commented Oct 27, 2021 •
edited
Loading

Copy link
Copy Markdown

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

🔨 Explore the source changes: 91b4c65

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

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

@netlify

netlify Bot commented Oct 27, 2021 •
edited
Loading

Copy link
Copy Markdown

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 91b4c65

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

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

@joshblack joshblack requested a review from dakahn October 27, 2021 16:43
@abbeyhrt

abbeyhrt commented Oct 27, 2021

Copy link
Copy Markdown
Contributor

What's the intended behavior for the right/left variants? For me, I would think bottom-left would mean the popover shows up in the bottom-left quadrant and same with top-left, top-right, etc but rather is it that if it's bottom-right, then the bottom right of the trigger and the bottom-right of the popover are aligned?

@joshblack joshblack requested a review from a team as a code owner October 27, 2021 20:54
@joshblack

joshblack commented Oct 27, 2021

Copy link
Copy Markdown
Contributor Author

@abbeyhrt yeah, the placement/alignment stuff is a little confusing. Basically the popover can be put above, below, or to other side. Then the other part is the caret alignment which so that:

  • Top: popover top, caret center
  • Top-left: popover top, caret left
  • Top-right: popover top, caret right
  • Right: popover right, caret center
  • Right-top: popover right, caret top
  • etc.

laurenmrice
laurenmrice requested changes Oct 28, 2021
View reviewed changes

@laurenmrice laurenmrice left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • The dropshadow should be 0 2px 6px 0 rgba(0, 0, 0, 0.2)

  • When a radio button under the "Caret Position" section is selected, the caret tip is not showing up in the storybook demo on the popover. Is that related to the changes in this PR?

@joshblack

joshblack commented Oct 28, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hey @laurenmrice! Which preview are you looking at? It should be:

https://deploy-preview-9965--carbon-react-next.netlify.app/?path=/story/experimental-unstable-popover--playground

The dropshadow should be 0 2px 6px 0 rgba(0, 0, 0, 0.2)

I couldn't figure out the right equivalent since this is using filter: drop-shadow which didn't seem to map to the box shadow properties in the spec. Here's a quick video going through the values 👀

Screen.Recording.2021-10-28.at.10.23.12.mov

If these aren't matching, we could use multiple shadows to try and emulate the effect but I wasn't sure what values would be needed. Let me know what you think!

@laurenmrice

laurenmrice commented Oct 28, 2021

Copy link
Copy Markdown
Member

@joshblack Oh, okay got it. I was not looking at the correct link. I will take a look again. Thanks!

@joshblack joshblack mentioned this pull request Oct 28, 2021
Closed
@joshblack

joshblack commented Oct 28, 2021

Copy link
Copy Markdown
Contributor Author

bump @aledavila @dakahn @abbeyhrt

aledavila
aledavila approved these changes Oct 28, 2021
View reviewed changes

@aledavila aledavila left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@joshblack joshblack mentioned this pull request Oct 29, 2021
Merged
laurenmrice
laurenmrice requested changes Nov 1, 2021
View reviewed changes

@laurenmrice laurenmrice left a comment •
edited by joshblack
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Storybook demo spec:

  • Instead of using a 48px container and a 32px icon for the trigger, lets use a 32px container and a 16px icon.
  • When the caret tip is set to true: there should be a 4px space between the caret and the container bounding box.
    storybook demo spec

Direction alignments:
When the caret tip is set to true, most of the alignments looks good, but some popovers just need a 2px nudge)

  • Top-left (move popover 2px to right)
  • Top-right (move popover 2px to left)
  • Bottom-left (move popover 2px to right)
  • Bottom-right (move popover 2px to left)
  • Left-bottom (move popover 2px up)
  • Left-top (move popover 2px down)
  • Right-bottom (move popover 2px up)
  • Right-top (move popover 2px down)

@joshblack joshblack requested a review from laurenmrice November 2, 2021 22:11
@joshblack

joshblack commented Nov 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@laurenmrice should be good to go for a re-review!

I tried to spend this afternoon tackling the caret alignment that you brought up and I think I got it so that the caret will automatically center regardless of the size of the trigger and the placement of the caret 🤞

It also was updated to include that 10px offset that we talked about plus the 12px x 6px triangle 🔥

This also works with the drop-shadow() stuff I was talking about so the caret and popover background are having the shadow applied to both elements instead of us having to hack around with the rotating box trick 😌

dakahn
dakahn approved these changes Nov 3, 2021
View reviewed changes

@dakahn dakahn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool!

laurenmrice
laurenmrice requested changes Nov 3, 2021
View reviewed changes

@laurenmrice laurenmrice left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • Directions look great in Safari/Chrome/Firefox! 🎉

  • The only thing we need to adjust now is that the 16px checkbox icon looks like it was enlarged to fill the 16px container around it. The true asset of the 16px icon has some padding around the artwork inside of the 16px container. https://carbon-elements.netlify.app/icons/examples/preview/#Checkbox16

  • Example of current code versus design spec:
    Artboard

@joshblack

joshblack commented Nov 3, 2021

Copy link
Copy Markdown
Contributor Author

@laurenmrice updated!

@joshblack joshblack requested a review from laurenmrice November 3, 2021 16:45
laurenmrice
laurenmrice approved these changes Nov 3, 2021
View reviewed changes

@laurenmrice laurenmrice left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great ! 🎉

@kodiakhq kodiakhq Bot merged commit 99baef6 into carbon-design-system:main Nov 3, 2021
@joshblack joshblack deleted the feat/update-popover branch November 3, 2021 20:22
@SimonFinney SimonFinney mentioned this pull request Nov 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laurenmrice laurenmrice laurenmrice approved these changes

+3 more reviewers

@aledavila aledavila aledavila approved these changes

@abbeyhrt abbeyhrt abbeyhrt approved these changes

@dakahn dakahn dakahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status: ready to merge 🎉

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@joshblack @abbeyhrt @laurenmrice @aledavila @dakahn