Add `preferredPosition` to polaris-popover #117

tomnez · 2018-04-23T18:51:24Z

Overview

This adds the preferredPosition to the polaris-popover component.

It basically translates the user-provided preferredPosition value into a verticalPosition attribute since we use ember-basic-dropdown as our popover component under the hood.

Test for this is skipped because ember-basic-dropdown uses ember wormhole for the popover content, which renders content outside of the ember testing container.

tomnez · 2018-04-23T18:52:59Z

addon/components/polaris-popover.js

 import layout from '../templates/components/polaris-popover';

+const { ViewUtils } = Ember;


Hax, suggested by @andrewpye - emberjs/rfcs#168 (comment)

I don't believe there is a way to import these utils using the ES6 modules syntax.

andrewpye

Just leaving some comments for now to start some discussion, although this does look alright IMO

andrewpye · 2018-04-24T08:19:26Z

addon/components/polaris-popover.js

   * @default null
-   * TODO: not implemented
   */
  preferredPosition: null,


This should probably default to below to match the React implementation

andrewpye · 2018-04-24T08:20:05Z

addon/components/polaris-popover.js

+    // and set when the user opens the popover.
+
+    return null;
+  }),


.readOnly()?

This gets set by the onOpen action if the position is set to "mostSpace", so it can't be readOnly

Hmm... seems like an unlikely scenario, but setting this in onOpen will mean that if the host app passes in a dynamic preferredPosition and that value changes from 'mostSpace' after first open, the popover won't honour that change because onOpen will overwrite the CP behaviour here. You could add an explicit setter to this CP, and that way I think that (really tiny) edge case would be avoided...

verticalPosition is still a CP, so if preferredPosition dynamically changes, it will re-set verticalPosition to the new value, and then when onOpen runs it will not compute a new value for verticalPosition because it only computes a new value if preferredPosition is set to "mostSpace".

After setting verticalPosition on line 161, my understanding is that verticalPosition will no longer be a CP - the set will replace the entire CP with the literal value that you're setting it to, unless the CP has its own set defined as well. Think of it like this, where someObject is the result of the computed call:

... other props... myComputed: someObject, ... other stuff...

If you then this.set('myComputed', 'something else'), you'll replace someObject with the new string literal. The CP "contained" in someObject will be torn down, and will no longer be computed on dependency changes. From then on, this.get('myComputed') will always return 'something else'.

You can work around this by defining the CP as

myComputed: computed('dep1', 'dep2', { get() { return 'whatever'; }, set(key, value) { return value; } ),

The set part ensures that the CP remains after a this.set('myComputed', 'something else'), and the value will still update after dep1 or dep2 change...

andrewpye · 2018-04-24T08:20:32Z

addon/components/polaris-popover.js

+
+    if (preferredPosition === 'below') {
+      return preferredPosition;
+    }


Why not bundle these two into a single if-statement?

andrewpye · 2018-04-24T08:21:32Z

addon/components/polaris-popover.js

+    let component;
+
+    // Hack to get the component's container
+    // element since this component uses `tagName: ''`


Add a link here to the RFC I linked you to so we know where this came from in the future 👍

(unless of course @vladucu knows any better way to do this?)

good with me for now, ideally I think we should investigate refactoring this at some point and not using ember-basic-dropdown

andrewpye · 2018-04-24T08:27:22Z

addon/components/polaris-popover.js

+      let preferredPosition = this.get('preferredPosition');
+
+      if (preferredPosition === 'mostSpace') {
+        this.set('verticalPosition', this.mostVerticalSpace());


Not a huge fan of the naming of mostVerticalSpace as a method - it sounds more like a property since there's no verb in the name. How about getMostVerticalSpace or making it a volatile CP and keeping the mostVerticalSpace name?

andrewpye · 2018-04-24T08:27:41Z

docs/popover.md

@@ -6,7 +6,7 @@
 component](https://polaris.shopify.com/components/overlays/popover). This component uses [`ember-basic-dropdown`](https://github.com/cibernox/ember-basic-dropdown) to implement popover functionality. Note that the usage is slightly different from the React implementation so please pay attention to the examples below.


-**NOTE:** _the `preferredPosition`, `active`, `activatorWrapper`, `preventAutofocus` and `onClose` properties are currently unimplemented._
+**NOTE:** _the `active`, `activatorWrapper`, `preventAutofocus` and `onClose` properties are currently unimplemented._


andrewpye · 2018-04-24T08:31:47Z

tests/integration/components/polaris-popover-test.js

@@ -101,3 +103,29 @@ test('it renders the correct HTML with sectioned attribute', function(assert) {
  assert.equal(popoverSections.length, 1, 'renders one popover section');
  assert.equal(popoverSections[0].textContent.trim(), 'This is some sectioned popover content', 'popover section contains the correct content');
 });
+
+skip('it renders the correct HTML with `prefferedPosition` attribute', function(assert) {


@vladucu what are your thoughts on this test? It's skipped at the moment because it's testing things rendered outside the test div - we can either keep it here and skip it, or maybe look into using document.querySelector etc. instead of the test helpers to retrieve the necessary elements?

yes, I'd lean towards using document.querySelector (or whatever works) and not skip the test if possible

👍 I'll see if document.querySelector works

andrewpye · 2018-04-24T08:32:22Z

tests/integration/components/polaris-popover-test.js

@@ -101,3 +103,29 @@ test('it renders the correct HTML with sectioned attribute', function(assert) {
  assert.equal(popoverSections.length, 1, 'renders one popover section');
  assert.equal(popoverSections[0].textContent.trim(), 'This is some sectioned popover content', 'popover section contains the correct content');
 });
+
+skip('it renders the correct HTML with `prefferedPosition` attribute', function(assert) {


Also @tomnez you've got a typo all over the place with preffered 😛

tomnez · 2018-04-24T14:30:23Z

@vladucu @andrewpye updated - also I left the controllers/test/index.js file in there because I have found myself repeatedly re-creating it to test new components. Any opposition to this?

tomnez · 2018-04-24T16:19:20Z

@andrewpye updated! And thanks for the detailed explanations 👍

andrewpye

Think this is good, one single-quote => double-quote change to make then 🚢 🔥

andrewpye · 2018-04-24T17:09:51Z

addon/templates/components/polaris-popover.hbs

@@ -1,4 +1,6 @@
 {{#basic-dropdown
+  verticalPosition=verticalPosition
+  onOpen=(action 'onOpen')


Double-quotes in template files pls 😉

andrewpye · 2018-04-24T17:10:21Z

tests/integration/components/polaris-date-picker-test.js

@@ -339,7 +339,7 @@ test('it does not fire actions when disabled days are clicked', function(assert)
    year: YEAR,
    selected: null,
    disableDatesAfter: DISABLE_AFTER,
-    onChange: (selected) => {
+    onChange: () => {


Not sure you need the colon here actually

andrewpye · 2018-04-24T17:10:47Z

tests/integration/components/polaris-popover-test.js

+
+  const contentBelow = document.querySelector(popoverContentBelowSelector);
+  assert.ok(contentBelow, 'renders popover content below the trigger');
+});


Love that you got this working 😍

tomnez · 2018-04-24T17:19:54Z

@andrewpye updated

tomnez added 3 commits April 23, 2018 11:38

Add preferredPosition attr to polaris-popover

45471e9

Update README

071ca53

Add skipped test for preferredPosition attribute

3915ec8

tomnez requested review from andrewpye and vladucu April 23, 2018 18:51

tomnez commented Apr 23, 2018

View reviewed changes

andrewpye assigned tomnez Apr 24, 2018

andrewpye reviewed Apr 24, 2018

View reviewed changes

PR edits and update skipped test so it runs

71589e2

andrewpye approved these changes Apr 24, 2018

View reviewed changes

Use consts for above/below, add set/get for positioning

ae90dda

tomnez force-pushed the add-positioning-to-polaris-popover branch from e4d261e to ae90dda Compare April 24, 2018 17:13

tomnez merged commit 8ffbab7 into master Apr 24, 2018

tomnez deleted the add-positioning-to-polaris-popover branch April 24, 2018 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `preferredPosition` to polaris-popover #117

Add `preferredPosition` to polaris-popover #117

tomnez commented Apr 23, 2018

tomnez Apr 23, 2018

andrewpye left a comment

andrewpye Apr 24, 2018

andrewpye Apr 24, 2018

tomnez Apr 24, 2018

andrewpye Apr 24, 2018

tomnez Apr 24, 2018 •

edited

andrewpye Apr 24, 2018

andrewpye Apr 24, 2018

andrewpye Apr 24, 2018

andrewpye Apr 24, 2018

vladucu Apr 24, 2018

andrewpye Apr 24, 2018

andrewpye Apr 24, 2018

andrewpye Apr 24, 2018

vladucu Apr 24, 2018

tomnez Apr 24, 2018

andrewpye Apr 24, 2018

tomnez Apr 24, 2018

tomnez commented Apr 24, 2018

tomnez commented Apr 24, 2018

andrewpye left a comment

andrewpye Apr 24, 2018

andrewpye Apr 24, 2018

andrewpye Apr 24, 2018

tomnez commented Apr 24, 2018

		import layout from '../templates/components/polaris-popover';

		const { ViewUtils } = Ember;

Add preferredPosition to polaris-popover #117

Add preferredPosition to polaris-popover #117

Conversation

tomnez commented Apr 23, 2018

Overview

Choose a reason for hiding this comment

andrewpye 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

tomnez Apr 24, 2018 • edited

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

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

tomnez commented Apr 24, 2018

tomnez commented Apr 24, 2018

andrewpye 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

tomnez commented Apr 24, 2018

Add `preferredPosition` to polaris-popover #117

Add `preferredPosition` to polaris-popover #117

tomnez Apr 24, 2018 •

edited