-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
0d1f710
to
26a3a9e
Compare
}} | ||
aria-label={{accessibilityLabel}} | ||
data-test-polaris-spinner | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change mean we can remove the SvgHandling
mixin from polaris-spinner
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yasss, let's remove that :D
'Integration | Component | polaris spinner', | ||
{ | ||
integration: true, | ||
module('Integration | Component | polaris spinner', function(hooks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌 Thanks for doing this! 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments and what already mentioned....but looks good otherwise
🚀
addon/components/polaris-spinner.js
Outdated
return `polaris/spinner-${this.get('normalizedSize')}`; | ||
spinnerSVG: computed('normalizedSize', function() { | ||
let size = this.get('normalizedSize') === 'large' ? 'large' : 'small'; | ||
return `/@smile-io/ember-polaris/illustrations/spinner-${size}.svg`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make a map with explicit image sizes, ex
const spinnerSVGSources = {
small: '/@smile-io/ember-polaris/illustrations/spinner-small.svg'
large: /@smile-io/ember-polaris/illustrations/spinner-large.svg
}
This is to avoid issues with fingerprinting
}} | ||
aria-label={{accessibilityLabel}} | ||
data-test-polaris-spinner | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yasss, let's remove that :D
No description provided.