feat(popup): base implementation #150

bmdalex · 2018-08-28T18:24:25Z

Popup

This PR introduces the Popup, a component that displays additional information on top of a page at the end of the body DOM element. It comes with the following props:

basic?: boolean: when true, it introduces basic CSS styling for the popup (default false)
align?: 'top' | 'bottom' | 'start' | 'end' | 'center'
position?: 'above' | 'below' | 'before' | 'after': the position of the popup
trigger: JSX.Element: the React component that controls the popup; for now, clicking it toggles the visibility of the popup

TODO

API Proposal

Default

A default popup requires the trigger prop and renders a popup at the end of the body DOM element

<Popup trigger={<Button content="Add" />} content="Add users to your feed" />

generates

<button>Add</button>

and at the end of body DOM element:

<body>
  ...
  <div>Add users to your feed</div>
</body>

Basic

A basic popup renders a popup with very basic CSS.

<Popup basic trigger={<Button content="Add" />} content="Add users to your feed" />

generates

<button>Add</button>

and at the end of body DOM element:

<body>
  ...
  <div>Add users to your feed</div>
</body>

Alignment and Position

A popup can be rendered in 12 different combinations of align and position props around the trigger element:

position="above" align="start":

LTR	RTL

position="above" align="center":

LTR	RTL

position="above" align="end":

LTR	RTL

position="below" align="start":

LTR	RTL

position="below" align="center":

LTR	RTL

position="below" align="end":

LTR	RTL

position="before" align="top":

LTR	RTL

position="before" align="center":

LTR	RTL

position="before" align="bottom":

LTR	RTL

position="after" align="top":

LTR	RTL

position="after" align="center":

LTR	RTL

position="after" align="bottom":

LTR	RTL

<Popup
  align="start"
  position="above"
  trigger={<Button content="Add" />}
  content="Add users to your feed"
/>

generates

<button>Add</button>

and at the end of body DOM element:

<body>
  ...
  <div>Add users to your feed</div>
</body>

layershifter · 2018-08-28T18:30:09Z

The positioning part of the Popup component is one of complicated things. Can we omit it and use PopperJS there?

/cc @levithomason

codecov · 2018-08-28T19:17:15Z

Codecov Report

Merging #150 into master will increase coverage by 0.92%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   90.17%   91.09%   +0.92%     
==========================================
  Files          55       59       +4     
  Lines         916     1011      +95     
  Branches      142      159      +17     
==========================================
+ Hits          826      921      +95     
  Misses         86       86              
  Partials        4        4

Impacted Files	Coverage Δ
src/index.ts	`100% <100%> (ø)`	⬆️
src/components/Popup/PopupContent.tsx	`100% <100%> (ø)`
src/components/Popup/positioningHelper.ts	`100% <100%> (ø)`
src/components/Popup/index.ts	`100% <100%> (ø)`
src/components/Popup/Popup.tsx	`100% <100%> (ø)`

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 7cde3dd...935d4ec. Read the comment docs.

bmdalex · 2018-08-28T19:31:33Z

@layershifter
agreed, RTL and handling viewport limits do not work in this implementation; I took a look at react-popper package and looks pretty cool.

@levithomason this is definitely something we want to try; do we want to address it in this PR or should we iterate?

levithomason · 2018-08-29T20:41:21Z

Popper looks extremely promising. It looks like the React wrapper is also gaining a lot of adoption:
https://www.npmjs.com/package/react-popper

Let's definitely do our due diligence here and compare what we'd gain and lose by switching. I would love nothing more than to off load the problem components, such as Popup, to a shared community work load.

Edit

Just saw your latest comment @Bugaa92, yes, let's try Popper now before merging.

src/components/Popup/Popup.tsx

sophieH29 · 2018-08-30T07:59:21Z

From the accessibility point of view, as a User, I would like to add Popup behavior which will have defined actions to open/close popup with specified keys

actionsDefinition: {
    trigger: {
      open: {
        keyCombinations: [{ keyCode: keyboardKey.Enter }, { keyCode: keyboardKey.Spacebar }, { keyCode: keyboardKey.ArrowDown }],
      },
    },
    portal: {
      close: {
        keyCombinations: [{ keyCode: keyboardKey.Escape }],
      },
    }
  }

And since as open/close logic is enclosed into the portal component, can we rethink its API to give a possibility to trigger open/close function from a popup? What your thoughts about it?

@Bugaa92 @levithomason @jurokapsiar

src/components/Popup/Popup.tsx

miroslavstastny · 2018-08-30T09:13:20Z

Also we should think about screener tests for this. With current examples, there is no screener coverage for the examples, because these are the screenshots we compare:

So we either need to add examples with popup initially displayed (which will not work for some variants like fullscreen) or better introduce Test integrations for Screener tests.

bmdalex · 2018-09-03T17:48:19Z

@levithomason
@kuzhelov
@mnajdova

what do you guys think about @miroslavstastny 's suggestions regarding visual tests? see his comment below, he has a very good point!

currently we don't seem to have the infrastructure to do this kind of scenario tests (that involve actions) and I'm afraid having the popups open by default are going to offer a bad UX

miroslavstastny

top-* and bottom-* positions are not flipped in RTL:

package.json

docs/src/examples/components/Popup/Variations/PopupExampleBasic.shorthand.tsx

src/components/Ref/Ref.tsx

miroslavstastny · 2018-09-07T06:30:22Z

src/components/Portal/Portal.tsx

+
+type ReactMouseEvent = React.MouseEvent<HTMLElement>
+
+export interface IPortalProps {


The interface does not match propTypes - as, children, styles, variables

thanks, fixed

miroslavstastny · 2018-09-07T07:27:12Z

src/components/Popup/Popup.tsx

+  after = 'after',
+}
+
+type Position =


Maybe it's just me, but I am still confused with the naming here. Let's discuss what the two parts of the position name mean in both RTL and LTR and agree on the naming in broader group.
I guess the main source of confusion is start/end meaning something different in combination with top/bottom vs before/after. I can see the inspiration in flexbox'es main/cross axis, but still...

yea, I'll add this to your meeting notes to discuss it with everybody

docs/src/examples/components/Popup/Variations/PopupExamplePosition.shorthand.tsx

src/components/Popup/PopupContent.tsx

layershifter · 2018-09-07T07:45:48Z

src/components/Ref/Ref.tsx

+  }
+
+  componentDidMount() {
+    _.invoke(this.props, 'innerRef', findDOMNode(this))


There was a condition in SUIR code, let's use it there, too

not sure it's necessary since _.invoke invokes innerRef function only if it's defined; isn't it the same as SUIR?

bmdalex · 2018-09-07T14:33:45Z

@miroslavstastny

I fixed RTL for all positions
I added UTs to cover these properly
I updated screenshots with new popup styles and RTL examples to make things more obvious

…at/popup-simple

test/specs/components/Popup/Popup-test.tsx

docs/src/examples/components/Popup/Variations/PopupExampleBasic.shorthand.tsx

miroslavstastny · 2018-09-10T07:56:51Z

src/components/Popup/Popup.tsx

+    const computedStyle = rtl ? rtlCSSJS(style) : style
+
+    return (
+      <Popup.Content innerRef={ref} basic={basic} styles={{ root: computedStyle }}>


RTL is broken in docsite as dir="rtl" is not set on body which is a parent of Popup.Content.
Do we want to add something like {...rtl && {dir:"rtl"}} here? @jurokapsiar

fixed, very good catch; I missed it because of using simple text as content (with no punctuation)

into feat/popup-simple

…at/popup-simple

miroslavstastny · 2018-09-11T10:17:20Z

src/components/Popup/positioningHelper.ts

+  const alignedVertically = a === 'top' || a === 'bottom'
+
+  return (
+    (isPositionedVertically && alignedVertically) || (!isPositionedVertically && !alignedVertically)


nit:

isPositionedVertically === alignedVertically

sophieH29 · 2018-09-11T12:55:59Z

@Bugaa92 I would add a description to each button in the example (with used props align="start" position="start") as you did in the PR's description. Because currently you're just guessing what position you'll get when clicking on a button :)

What do you think?

…at/popup-simple

bmdalex requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners August 28, 2018 18:24

bmdalex force-pushed the feat/popup-simple branch from 73e67c1 to 82cd7c2 Compare August 28, 2018 19:11

bmdalex added help wanted Extra attention is needed question Further information is requested, concerns that require additional thought are raised and removed help wanted Extra attention is needed labels Aug 28, 2018

bmdalex changed the title ~~Feat/popup simple~~ feat(popup): base implementation Aug 29, 2018

bmdalex force-pushed the feat/popup-simple branch from 82cd7c2 to 95771c5 Compare August 29, 2018 09:26

levithomason added needs author feedback Author's opinion is asked and removed question Further information is requested, concerns that require additional thought are raised labels Aug 29, 2018

sophieH29 reviewed Aug 30, 2018

View reviewed changes

src/components/Popup/Popup.tsx Outdated Show resolved Hide resolved

sophieH29 reviewed Aug 30, 2018

View reviewed changes

src/components/Popup/Popup.tsx Outdated Show resolved Hide resolved

bmdalex force-pushed the feat/popup-simple branch from 95771c5 to 37d92cc Compare September 3, 2018 17:38

bmdalex force-pushed the feat/popup-simple branch 3 times, most recently from ee3da57 to ed34c98 Compare September 4, 2018 09:35

sophieH29 mentioned this pull request Sep 4, 2018

[RFC] Extendable Portal functionality #180

Closed

bmdalex force-pushed the feat/popup-simple branch 3 times, most recently from 597ff68 to 00f6ef0 Compare September 6, 2018 11:58

miroslavstastny suggested changes Sep 7, 2018

View reviewed changes

layershifter reviewed Sep 7, 2018

View reviewed changes

miroslavstastny added needs author changes Author needs to implement changes before merge and removed 💥 blocked help wanted Extra attention is needed labels Sep 7, 2018

bmdalex added 2 commits September 7, 2018 15:56

feat(portal): simple base implementation

50bda74

addressed PR comments

d0df7ed

bmdalex force-pushed the feat/popup-simple branch from fcd7b07 to a5dc328 Compare September 7, 2018 14:32

bmdalex added 2 commits September 7, 2018 18:28

feat(popup): simple base implementation

f89f9ec

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

01710c7

…at/popup-simple

bmdalex force-pushed the feat/popup-simple branch from a5dc328 to 01710c7 Compare September 7, 2018 16:39

addressed comments

7b9e89e

miroslavstastny reviewed Sep 10, 2018

View reviewed changes

bmdalex added 2 commits September 10, 2018 11:58

addressed comments

a1da665

Merge branch 'feat/popup-simple' of https://github.com/stardust-ui/react

0b800d0

into feat/popup-simple

miroslavstastny approved these changes Sep 10, 2018

View reviewed changes

bmdalex mentioned this pull request Sep 10, 2018

[RFC] Popup position #210

Closed

bmdalex added 2 commits September 11, 2018 09:15

addressed comments

8233bf0

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

2ec0fae

…at/popup-simple

bmdalex force-pushed the feat/popup-simple branch from f25dd64 to 2ec0fae Compare September 11, 2018 07:19

fixed unit tests

935d4ec

bmdalex force-pushed the feat/popup-simple branch from ce91227 to 935d4ec Compare September 11, 2018 09:28

miroslavstastny approved these changes Sep 11, 2018

View reviewed changes

bmdalex added 3 commits September 11, 2018 15:15

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

e1e7eeb

…at/popup-simple

improved docs and examples

5f215bb

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

c044a93

…at/popup-simple

bmdalex merged commit b851b3d into master Sep 11, 2018

bmdalex deleted the feat/popup-simple branch September 11, 2018 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(popup): base implementation #150

feat(popup): base implementation #150

bmdalex commented Aug 28, 2018 •

edited

Loading

layershifter commented Aug 28, 2018 •

edited

Loading

codecov bot commented Aug 28, 2018 •

edited

Loading

bmdalex commented Aug 28, 2018

levithomason commented Aug 29, 2018 •

edited

Loading

sophieH29 commented Aug 30, 2018

miroslavstastny commented Aug 30, 2018

bmdalex commented Sep 3, 2018

miroslavstastny left a comment

miroslavstastny Sep 7, 2018

bmdalex Sep 7, 2018

miroslavstastny Sep 7, 2018

bmdalex Sep 7, 2018

layershifter Sep 7, 2018

bmdalex Sep 7, 2018

bmdalex commented Sep 7, 2018

miroslavstastny Sep 10, 2018

bmdalex Sep 10, 2018

miroslavstastny Sep 11, 2018

sophieH29 commented Sep 11, 2018


		type ReactMouseEvent = React.MouseEvent<HTMLElement>

		export interface IPortalProps {

feat(popup): base implementation #150

feat(popup): base implementation #150

Conversation

bmdalex commented Aug 28, 2018 • edited Loading

Popup

TODO

API Proposal

Default

Basic

Alignment and Position

layershifter commented Aug 28, 2018 • edited Loading

codecov bot commented Aug 28, 2018 • edited Loading

Codecov Report

bmdalex commented Aug 28, 2018

levithomason commented Aug 29, 2018 • edited Loading

sophieH29 commented Aug 30, 2018

miroslavstastny commented Aug 30, 2018

bmdalex commented Sep 3, 2018

miroslavstastny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmdalex commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophieH29 commented Sep 11, 2018

bmdalex commented Aug 28, 2018 •

edited

Loading

layershifter commented Aug 28, 2018 •

edited

Loading

codecov bot commented Aug 28, 2018 •

edited

Loading

levithomason commented Aug 29, 2018 •

edited

Loading