feat(Popup): add `pointing` prop by layershifter · Pull Request #1198 · microsoft/fluent-ui-react

layershifter · 2019-04-10T07:35:30Z

This PR adds a new pointing prop that allows to show a pointer to a trigger.

`pointing`

We have pointing prop in Menu, used this name for consistency.

Changes in `PopupContent`

DOM structure

WARNING: `arrow.element` must be child of its popper element!

A pointer should be a slot of PopupContent, otherwise it fill fail. Bootstrap uses PopperJS and the same structure, BTW.

`content` slot

As I have to change DOM structure, I added a new slot content, but it's really weird to have PopupContent.content 😮 inner may be?

…om/stardust-ui/react into feat/popup-pointing

layershifter · 2019-04-10T09:54:31Z

packages/react/src/themes/teams/components/Popup/popupStyles.ts

    position: 'absolute',
    textAlign: 'left',
-    color: variables.contentColor,
-    background: variables.contentBackgroundColor,


Moved to popupContentStyles...

Talk to @kuzhelov. In #1121 he intentionally moved the styles from popupContentStyles to here.

these styles should be left here, as we popup expects any content to be rendered with this set of styles, not just PopupContent. Also, please, ensure that PopupContent just CSS-inherits this set of colors.

These two requirements are necessary for introducing correct semantics around these styles, as well as for expected effects of theme switching.

FYI #1121

bmdalex

👍 just couple of comments

packages/react/src/themes/teams/components/Popup/popupContentStyles.ts

packages/react/src/themes/teams/components/Popup/popupContentVariables.ts

packages/react/src/themes/teams/components/Popup/popupContentStyles.ts

…om/stardust-ui/react into feat/popup-pointing

miroslavstastny

RTL is broken:

codecov · 2019-04-11T13:07:18Z

Codecov Report

Merging #1198 into master will decrease coverage by 0.03%.
The diff coverage is 60.86%.

@@            Coverage Diff            @@
##           master   #1198      +/-   ##
=========================================
- Coverage   82.54%   82.5%   -0.04%     
=========================================
  Files         751     751              
  Lines        8850    8865      +15     
  Branches     1246    1251       +5     
=========================================
+ Hits         7305    7314       +9     
- Misses       1530    1536       +6     
  Partials       15      15

Impacted Files	Coverage Δ
...es/teams/components/Popup/popupContentVariables.ts	`66.66% <ø> (ø)`	⬆️
...t/src/themes/teams/components/Popup/popupStyles.ts	`33.33% <ø> (ø)`	⬆️
packages/react/src/components/Popup/Popup.tsx	`76.43% <100%> (+0.63%)`	⬆️
...hemes/teams/components/Popup/popupContentStyles.ts	`20% <11.11%> (-20%)`	⬇️
...ckages/react/src/components/Popup/PopupContent.tsx	`96.77% <85.71%> (-3.23%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef5ab15...58f4fe0. Read the comment docs.

codecov · 2019-04-11T13:07:18Z

Codecov Report

Merging #1198 into master will decrease coverage by 0.04%.
The diff coverage is 58.33%.

@@            Coverage Diff             @@
##           master    #1198      +/-   ##
==========================================
- Coverage   82.57%   82.53%   -0.05%     
==========================================
  Files         752      751       -1     
  Lines        8868     8880      +12     
  Branches     1186     1259      +73     
==========================================
+ Hits         7323     7329       +6     
- Misses       1530     1536       +6     
  Partials       15       15

Impacted Files	Coverage Δ
...rc/themes/teams/components/Popup/popupVariables.ts	`100% <ø> (ø)`	⬆️
...c/themes/teams-high-contrast/componentVariables.ts	`100% <ø> (ø)`	⬆️
.../react/src/themes/teams-dark/componentVariables.ts	`100% <ø> (ø)`	⬆️
...es/teams/components/Popup/popupContentVariables.ts	`66.66% <ø> (ø)`	⬆️
...t/src/themes/teams/components/Popup/popupStyles.ts	`33.33% <0%> (ø)`	⬆️
packages/react/src/components/Popup/Popup.tsx	`76.43% <100%> (+0.63%)`	⬆️
...hemes/teams/components/Popup/popupContentStyles.ts	`20% <11.11%> (-20%)`	⬇️
...ckages/react/src/components/Popup/PopupContent.tsx	`96.77% <85.71%> (-3.23%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8dd9a6...c504a33. Read the comment docs.

…om/stardust-ui/react into feat/popup-pointing

miroslavstastny · 2019-04-11T14:11:20Z

packages/react/src/themes/teams/components/Popup/popupContentVariables.ts

+  borderSize: string
  padding: string
+
+  contentColor: string


there are contentColor and contentBackgroundColor left in popupVariables as well and are not used in popupStyles

popupVariablesin teams-dark is setting color which is not used

if we move variables from popup to popupContent it is a breaking change

contentColor and contentBackgroundColor are now variables of PopupContent. Is the content in the variables' names still meaningful?

miroslavstastny · 2019-04-12T09:12:28Z

docs/src/examples/components/Popup/Types/PopupExamplePointing.shorthand.tsx

+const PopupWithButton = props => (
+  <Popup
+    align={props.align}
+    content="A popup with a pointer."


For the other popup example, we intentionally use multiline content to clearly show the difference between top/center/bottom - it would be great to have the same here as well.

kuzhelov

please, take a look on this comment about styles: https://github.com/stardust-ui/react/pull/1198/files#r273879017

…at/popup-pointing

…om/stardust-ui/react into feat/popup-pointing

feat(Popup): add pointing prop

e620c1e

layershifter requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners April 10, 2019 07:35

layershifter added ⚙️ enhancement New feature or request 🚧 WIP labels Apr 10, 2019

layershifter added 3 commits April 10, 2019 12:12

Merge branches 'feat/popup-pointing' and 'master' of https://github.c…

bc05e57

…om/stardust-ui/react into feat/popup-pointing

simplify styles

89e267e

polish styles

7c2811f

layershifter commented Apr 10, 2019

View reviewed changes

layershifter changed the title ~~[WIP] feat(Popup): add pointing prop~~ feat(Popup): add pointing prop Apr 10, 2019

layershifter added 🚀 ready for review and removed 🚧 WIP labels Apr 10, 2019

bmdalex reviewed Apr 10, 2019

View reviewed changes

layershifter added 2 commits April 11, 2019 12:29

Merge branches 'feat/popup-pointing' and 'master' of https://github.c…

9cf8eef

…om/stardust-ui/react into feat/popup-pointing

wip

7a3439f

miroslavstastny reviewed Apr 11, 2019

View reviewed changes

layershifter added 2 commits April 11, 2019 15:20

rework variables and fix RTL

35761cf

fix UTs

58f4fe0

layershifter added 2 commits April 11, 2019 16:16

Merge branches 'feat/popup-pointing' and 'master' of https://github.c…

42068f8

…om/stardust-ui/react into feat/popup-pointing

add changelog entry

5d9a56b

miroslavstastny reviewed Apr 11, 2019

View reviewed changes

layershifter added 4 commits April 11, 2019 17:26

rework styles

5f603c5

remove cruft variables

9a933e2

remove cruft, add breaking change for colors

827fc97

reduce to pointerSize

ba949d5

miroslavstastny approved these changes Apr 12, 2019

View reviewed changes

kuzhelov suggested changes Apr 12, 2019

View reviewed changes

wip2

bba2d9e

kuzhelov approved these changes Apr 12, 2019

View reviewed changes

layershifter mentioned this pull request Apr 12, 2019

test(Popup): add visual tests for multiple themes #1216

Merged

layershifter added 5 commits April 12, 2019 14:31

Merge branch 'master' of https://github.com/stardust-ui/react into fe…

6473751

…at/popup-pointing

rework border

6be1f24

replace padding with margin

2cacc2b

fix backrground

e89fc09

Merge branches 'feat/popup-pointing' and 'master' of https://github.c…

c504a33

…om/stardust-ui/react into feat/popup-pointing

layershifter merged commit 0f69933 into master Apr 15, 2019

delete-merged-branch bot deleted the feat/popup-pointing branch April 15, 2019 08:38

alinais mentioned this pull request Apr 18, 2019

A pointing UI for adding a beak from pop to the corresponding button #1172

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Popup): add `pointing` prop#1198

feat(Popup): add `pointing` prop#1198
layershifter merged 20 commits intomasterfrom
feat/popup-pointing

layershifter commented Apr 10, 2019 •

edited

Loading

Uh oh!

layershifter Apr 10, 2019

Uh oh!

miroslavstastny Apr 11, 2019

Uh oh!

kuzhelov Apr 12, 2019 •

edited

Loading

Uh oh!

bmdalex left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miroslavstastny left a comment

Uh oh!

codecov bot commented Apr 11, 2019

Uh oh!

codecov bot commented Apr 11, 2019 •

edited

Loading

Uh oh!

miroslavstastny Apr 11, 2019

Uh oh!

miroslavstastny Apr 12, 2019

Uh oh!

miroslavstastny Apr 12, 2019

Uh oh!

kuzhelov left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

layershifter commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pointing

Changes in PopupContent

DOM structure

content slot

Uh oh!

layershifter Apr 10, 2019

Choose a reason for hiding this comment

Uh oh!

miroslavstastny Apr 11, 2019

Choose a reason for hiding this comment

Uh oh!

kuzhelov Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmdalex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miroslavstastny left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 11, 2019

Codecov Report

Uh oh!

codecov bot commented Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

miroslavstastny Apr 11, 2019

Choose a reason for hiding this comment

Uh oh!

miroslavstastny Apr 12, 2019

Choose a reason for hiding this comment

Uh oh!

miroslavstastny Apr 12, 2019

Choose a reason for hiding this comment

Uh oh!

kuzhelov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

layershifter commented Apr 10, 2019 •

edited

Loading

`pointing`

Changes in `PopupContent`

`content` slot

kuzhelov Apr 12, 2019 •

edited

Loading

codecov bot commented Apr 11, 2019 •

edited

Loading