Update button to Polaris v3.10.0 #291
Conversation
| `href` attribute instead of replacing the whole element. | ||
| --}} | ||
| <a id={{id}} class={{classes}} aria-label={{accessibilityLabel}} data-test-id={{dataTestId}}> | ||
| {{#if hasBlock}} |
There was a problem hiding this comment.
Does
do the same thing as
if there isn't a block to yield? If so maybe we don't need the hasBlock check.
There was a problem hiding this comment.
I'd imagine most of the time, yes, but if contentComponent has different behaviour depending on whether it's in block or inline mode then we need to render it in either block or inline mode as appropriate. This is the case with quite a number of the components in ember-polaris, which yield if they're used in block mode, or render their text attribute otherwise.
I made a twiddle to show what I mean: both instance of outer-component cause inner-component to be rendered in inline mode - even if outer-component is rendered in block mode. It's a nuisance as your suggested simplification would be very nice, but unfortunately has breaking behaviour (which IIRC we've hit in the early days of ember-polaris) 😞
|
Will wait for @vladucu's 👀 on this before merging cos this is a big and potentially breaking refactor 😅 |
| {{#if url}} | ||
| {{#if isDisabled}} | ||
| {{!-- | ||
| Render an `<a>` so toggling disabled/enabled state changes only the |
There was a problem hiding this comment.
is this really a problem in Ember...or is it more to keep implementation as close as possible?
There was a problem hiding this comment.
The latter - I don't see any real disadvantage to having this implemented and it makes sure we're matching the React implementation closely, which can't hurt 🤷♂️
| await render(hbs`{{polaris-button url=mockUrl}}`); | ||
| assert | ||
| .dom('a[data-polaris-unstyled]') | ||
| .hasAttribute('href', this.get('mockUrl')); |
There was a problem hiding this comment.
no biggie (or need to update), but we should really start to use ES6 getters
There was a problem hiding this comment.
Do ES6 getters work with older versions of Ember (e.g. 2.18, which we still support)? Or do they cause any potential issues with apps that use older versions of Ember?
| assert.dom('button').hasText(this.get('label')); | ||
| }); | ||
|
|
||
| test('renders the given children into the link', async function(assert) { |
There was a problem hiding this comment.
children is really react-world.... 😜
block-form ?
There was a problem hiding this comment.
🤷♂️I think we all know what this means, and matching the React code makes the upgrade process much clearer (as I'm finding out this time around 😬)
Adds
monochrome,downloadandisPressedproperties topolaris-button. Also, to make future updates easier, I've rewrittenpolaris-buttonfrom scratch as a more-or-less direct port of the React implementation 🤷♂️ This will need careful and thorough testing in our apps to ensure the rewrite hasn't broken anything, although all our oldpolaris-buttontests still passed before I replaced them with the React tests.