Upgrade v2.11.0 choicelist #193

tomnez · 2018-10-08T15:28:00Z

Finishes #188

andrewpye

Great start, although I think we're missing an id attribute on the fieldset and I feel there's a little bit of room for making prop names more understandable

andrewpye · 2018-10-09T07:27:52Z

addon/components/polaris-choice-list.js

+  /**
+   * @private
+   */
+  hasError: bool('error'),


Not sure it's a big deal, but the React implementation has this as error != null so maybe we should look at mimicking that behaviour here?

andrewpye · 2018-10-09T07:28:13Z

addon/components/polaris-choice-list.js

+   *
+   * @property error
+   * @public
+   * @type {string|component}


These are from a time when I was young and foolish so I know this isn't your doing, but these types should be capitalised since they're class names

andrewpye · 2018-10-09T07:29:39Z

addon/components/polaris-choice-list.js

+   * @property error
+   * @public
+   * @type {string|component}
+   * @default false


This @default doesn't match the actual default value 😉

andrewpye · 2018-10-09T07:32:28Z

addon/components/polaris-choice-list.js

+  /**
+   * @private
+   */
+  finalNameError: computed('finalName', 'error', function() {


Wondering if this prop name is a bit unclear - what do you think about using something like ariaDescribedBy?

Good call, I'm going to also update hasError to ariaInvalid since it's only used for the aria-invalid attribute binding 👍

andrewpye · 2018-10-09T07:34:16Z

addon/components/polaris-choice-list.js

+      return null;
+    }
+
+    return `${this.get('finalName')}Error`;


We have an existing util for generating this, maybe worth using that here? 🤷‍♂️

addon/components/polaris-choice-list.js

vladucu

Tom, nice!!

Almost there, mostly Andy's comment and some minor ones on my end....was thinking, while we're here what do you think of cleaning up tests too: no longer use ember-native-dom-helpers (use same stuff provided by @ember/test-helpers), use test selectors instead of classes

vladucu · 2018-10-10T09:44:37Z

CHANGELOG.md

@@ -8,7 +8,7 @@
 - [#187](https://github.com/smile-io/ember-polaris/pull/187) [FEATURE] Add `disabled` attribute to `polaris-choice`
 - [#185](https://github.com/smile-io/ember-polaris/pull/185) [FEATURE] Update `polaris-button` to accept components for `icon` value
 - [#191](https://github.com/smile-io/ember-polaris/pull/191) [ENHANCEMENT] Update `polaris-layout/annotated-section` styling to match Polaris v2.11.0
-
+- [#193](https://github.com/smile-io/ember-polaris/pull/193) [FEATURE] Update `polaris-choice-list` to render `error` and `aria` attributes


this is not a new feature 😜...update? (a few others here are marked as feature when they shouldn't, ex: button)

vladucu · 2018-10-10T09:45:30Z

addon/components/polaris-choice-list.js

+
+  attributeBindings: [
+    'hasError:aria-invalid',
+    'finalNameError:aria-described-by',


should this be aria-describedby?

tomnez · 2018-10-10T15:24:17Z

@andrewpye @vladucu updated, had some conflicts so github might not be showing "since you last viewed" - the edits are all in this commit if you want to check it on its own.

andrewpye · 2018-10-11T06:31:28Z

addon/components/polaris-choice-list.js

+  ariaDescribedBy: computed('finalName', 'error', function() {
+    if (isNone(this.get('error'))) {
+      return null;
+    }


This check doesn't seem to be in the original, any reason for adding it?

Ahh no, I think this is a combination of my brain going rogue as well as a hilarious misunderstanding of what aria-describedby is for 🤦‍♂️

tomnez · 2018-10-11T18:50:17Z

@vladucu @andrewpye updated with edits, as well as updated the test file to use built-in helpers and ember-test-selectors 👍

andrewpye

I think this is all good @tomnez! 🔥

vladucu

just a couple of small comments re testing...otherwise looks good to me

vladucu · 2018-10-12T09:05:53Z

addon/templates/components/polaris-checkbox.hbs

@@ -9,6 +9,7 @@
 }}
  <span class="Polaris-Checkbox {{if error "Polaris-Checkbox--error"}}">
    <input
+      data-test-polaris-checkbox-input


to be consistent with other places....let's rename this to omit polaris, just data-test-checkbox-input

vladucu · 2018-10-12T09:08:31Z

addon/templates/components/polaris-choice-list.hbs

@@ -20,3 +20,12 @@
    </li>
  {{/each}}
 </ul>
+
+{{#if error}}
+  <div data-test-polaris-choice-list-error class="Polaris-ChoiceList__ChoiceError">


same, remove polaris

addon/components/polaris-choice-list.js

vladucu · 2018-10-12T09:11:09Z

addon/components/polaris-choice/label.js

@@ -66,4 +66,6 @@ export default Component.extend({
   * @public
   */
  disabled: null,
+
+  'data-test-polaris-choice': true,


same as the comment above

vladucu · 2018-10-12T09:11:31Z

tests/integration/components/polaris-choice-list-test.js

+
+    const choices = findAll(choiceSelector);
+    const radioInputs = findAll(radioInputSelector);
+    assert.equal(choices.length, 3, 'renders three choices');


you can just do `assert.dom(coiceSelector).exists({ count: 3 }, 'renders three choices');

vladucu · 2018-10-12T09:12:45Z

tests/integration/components/polaris-choice-list-test.js

+    assert.equal(choices.length, 3, 'renders three choices');
+
+    // Check the choices.
+    let choice = choices[0];


no need to use these anymore, you can just use assert.dom(${choiceSelector}:nth-child(1))` to target first item and so on

The nth selectors don't work in these cases because the choice selectors aren't all children of the same parent, so the test will only ever find nth-child(1) or nth-of-type(1) and then bork on trying to find the 2nd or 3rd, etc.

vladucu · 2018-10-12T09:13:17Z

tests/integration/components/polaris-choice-list-test.js

+    assert
+      .dom(choice)
+      .hasText('First option', 'first choice - has correct label text');
+    let radioInput = radioInputs[0];


similar as above for the input

tomnez · 2018-10-12T14:40:42Z

@vladucu made most of your edits, I updated selectors where I could but left a comment regarding the nth selectors.

vladucu

cool, don't think it's that important as long as we test it, I just personally like more working with the dom API

PS: the nth stuff likely didn't worked because you had to scope to the parent

tomnez force-pushed the upgrade-v2.11.0-choicelist branch from 4ca288a to c56e86c Compare October 8, 2018 15:30

tomnez mentioned this pull request Oct 8, 2018

Update polaris-choicelist component #188

Closed

tomnez requested review from vladucu and andrewpye October 8, 2018 15:31

tomnez self-assigned this Oct 8, 2018

andrewpye requested changes Oct 9, 2018

View reviewed changes

vladucu reviewed Oct 10, 2018

View reviewed changes

tomnez added 5 commits October 10, 2018 11:21

Upgrade choice-list to include error and aria attrs

df92109

Prettier format test file

17a74a6

Fix typo

1988acb

Update CHANGELOG

c408c86

PR edits

752389a

tomnez force-pushed the upgrade-v2.11.0-choicelist branch from c56e86c to 752389a Compare October 10, 2018 15:22

Linter updates

9c5208f

andrewpye reviewed Oct 11, 2018

View reviewed changes

tomnez added 3 commits October 11, 2018 12:43

Remove check for error in describedBy

2a67999

Update tests to use ember helpers

986e09e

Use ember-test-selectors in tests

1f81004

tomnez requested review from andrewpye and vladucu October 11, 2018 18:50

Fix linter error

38e9db8

andrewpye approved these changes Oct 12, 2018

View reviewed changes

vladucu requested changes Oct 12, 2018

View reviewed changes

PR edits

d30b88f

vladucu approved these changes Oct 12, 2018

View reviewed changes

tomnez merged commit 552635a into dev Oct 12, 2018

delete-merged-branch bot deleted the upgrade-v2.11.0-choicelist branch October 12, 2018 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade v2.11.0 choicelist #193

Upgrade v2.11.0 choicelist #193

tomnez commented Oct 8, 2018

andrewpye left a comment

andrewpye Oct 9, 2018

andrewpye Oct 9, 2018

andrewpye Oct 9, 2018

andrewpye Oct 9, 2018

tomnez Oct 10, 2018

andrewpye Oct 9, 2018

vladucu left a comment

vladucu Oct 10, 2018

vladucu Oct 10, 2018

tomnez commented Oct 10, 2018

andrewpye Oct 11, 2018

tomnez Oct 11, 2018

tomnez commented Oct 11, 2018

andrewpye left a comment

vladucu left a comment

vladucu Oct 12, 2018

vladucu Oct 12, 2018

vladucu Oct 12, 2018

vladucu Oct 12, 2018

vladucu Oct 12, 2018

tomnez Oct 12, 2018

vladucu Oct 12, 2018

tomnez commented Oct 12, 2018

vladucu left a comment

Upgrade v2.11.0 choicelist #193

Upgrade v2.11.0 choicelist #193

Conversation

tomnez commented Oct 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladucu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomnez commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomnez commented Oct 11, 2018

andrewpye left a comment

Choose a reason for hiding this comment

vladucu 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

Choose a reason for hiding this comment

tomnez commented Oct 12, 2018

vladucu left a comment

Choose a reason for hiding this comment