Add nowrap abilities to Polaris Stack #70
Conversation
addon/components/polaris-stack.js
Outdated
| @@ -15,6 +15,7 @@ export default Component.extend({ | |||
| 'spacingClassName', | |||
| 'alignmentClassName', | |||
| 'distributionClassName', | |||
| 'noWrapClassName' | |||
There was a problem hiding this comment.
Comma at the end here for consistency 😉
addon/components/polaris-stack.js
Outdated
| @@ -67,6 +68,15 @@ export default Component.extend({ | |||
| */ | |||
| distribution: 'baseline', | |||
|
|
|||
| /** | |||
| * Adjust the elements dont wrap | |||
There was a problem hiding this comment.
Been using the Polaris docs for these descriptions, so can you update this to Wrap stack elements to additional rows as needed on small screens (Defaults to true) please?
addon/components/polaris-stack.js
Outdated
| /** | ||
| * Adjust the elements dont wrap | ||
| * | ||
| * @property nowrap |
There was a problem hiding this comment.
The idea behind ember-polaris is to match the interfaces and behaviours of the official Polaris React components as closely as possible in Ember-land. With that in mind, flip the naming of this to wrap to match their interface, and default it to true please 🙂
There was a problem hiding this comment.
Ah, will do. I was going off the class name Polaris-Stack--noWrap :P
addon/components/polaris-stack.js
Outdated
| } | ||
|
|
||
| return 'Polaris-Stack--noWrap'; | ||
| }) |
There was a problem hiding this comment.
lol, this needs a comma here otherwise it won't build 😝
addon/components/polaris-stack.js
Outdated
| @@ -97,6 +107,15 @@ export default Component.extend({ | |||
| return `Polaris-Stack--distribution${classify(distribution)}`; | |||
| }).readOnly(), | |||
|
|
|||
| noWrapClassName: computed('noWrap', function() { | |||
| const noWrap = this.get('noWrap'); | |||
| if (isBlank(noWrapClassName) || noWrap === true) { | |||
There was a problem hiding this comment.
Not sure where this noWrapClassName is defined... this whole CP can be reduced to return this.get('wrap') === false ? null : 'Polaris-Stack--noWrap' to mimic the official React component's behaviour.
| this.render(hbs`{{polaris-stack noWrap=noWrap}}`); | ||
|
|
||
| const stack = find(stackSelector); | ||
| assert.equal(stack.className.indexOf('Polaris-Stack--noWrap'), -1, 'noWrap=false - does not add any noWrap class'); |
There was a problem hiding this comment.
Good tests 👍Minor comment, I'd prefer to see a more explicit assert like assert.notOk(stack.classList.contains('Polaris-Stack--noWrap'), /* description */) here, just because it's more obvious what the intent behind them is.
|
@vladucu now I've had some involvement in this, can I get your 👀 on it please? 😄 |
andrewpye
force-pushed
the
feature/polaris-stack-nowrap
branch
from
76ad830
to
9729d34
Compare
andrewpye
dismissed
their stale review
I've updated the things I requested be changed in my original review
|
Closing this |
|
@kmcgaire fair, we can reopen it once we update the Polaris version (we're looking into it, should get around to it soon) 👍 |
|
Reopened to merge into our Polaris v1.10.2 upgrade branch 👍 |
addon/components/polaris-stack.js
Outdated
| @@ -15,6 +15,7 @@ export default Component.extend({ | |||
| 'spacingClassName', | |||
| 'alignmentClassName', | |||
| 'distributionClassName', | |||
| 'noWrapClassName', | |||
There was a problem hiding this comment.
I think this can be simplified to just
wrap::Polaris-Stack--noWrap
notice double :
There was a problem hiding this comment.
@vladucu nope, that breaks the desired behaviour that the noWrap class is only applied if wrap is explicitly false:
There was a problem hiding this comment.
makes sense....could we simplify to something like
classNameBindings: ['noWrap:Polaris-Stack--noWrap'],
noWrap: eq('wrap', false)
Add polaris functionality to add noWrap classes to Polaris Stack