-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accept card title as a string or a component #152
Conversation
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.
Nice updates 🙂 Could maybe use some tweaking to play nicely with #147 though?
|
||
await render(hbs`{{is-component-definition inputValue}}`); | ||
|
||
assert.equal(this.element.textContent.trim(), '1234'); |
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.
This test needs updating, surely it doesn't pass?
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.
many tests should be updated if we want to be consistent with latest ones...though this should work well (ember always has backwards support)
} | ||
|
||
let contentConstructorName = content.constructor.name || ''; | ||
return contentConstructorName.indexOf('ComponentDefinition') > -1; |
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.
Should probably update render-content
to use this function to keep things DRY 👍
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.
yeah, can do
@@ -3,7 +3,11 @@ | |||
<div class="Polaris-Card__Header"> | |||
{{#polaris-stack alignment="baseline" as |stack|}} | |||
{{#stack.item fill=true}} | |||
{{polaris-heading text=title}} | |||
{{#if (is-component-definition title)}} |
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.
Hmm... this helper is a nice idea, but it will ignore the changes I made to render-content
in #147 (handling content
being a hash of componentName
and props
). We probably want to be able to support both the component definition and componentName
+ props
hash way of doing things here, since render-content
will support it...
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.
not really an idea...we need to not wrap title in a heading if it's passed as a component....but will check if we can support both
@@ -62,6 +63,7 @@ export default Component.extend({ | |||
* @property headerActions | |||
* @type {Action[]} |
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.
This should probably get changed to Object[]
now, maybe worth checking if any other @type
s need updating too
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.
will leave for when we cleanup/refactor...just cause there are many things we'd need to update here
@andrewpye updated, dry-ed up the |
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.
Looks good @vladucu 🔥
No description provided.