-
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
Implement Polaris inline-error component #174
Conversation
12d43b9
to
da1b4bb
Compare
|
||
init() { | ||
this._super(...arguments); | ||
assert('[polaris-inline-error] Missing required `fieldID` param!', this.get('fieldID')); |
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.
can we start using this.fieldID
instead of this.get
?
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.
was bitten by this the other day...it won't work in apps on pre-ember 3.1
is there any way to support older versions of Ember too in the addon?
<div | ||
class="Polaris-InlineError" | ||
id={{concat fieldID "Error"}} | ||
data-test-inline-error={{data-test-inline-error}} |
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.
If we had it as,
<div
data-test-inline-error
class="Polaris-InlineError"
id={{concat fieldID "Error"}}
></div>
then we wouldn't need to set the same on the class as boolean flags. You could find the usage here.
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.
yes, but in a consuming app, in an acceptance test where you have multiple instances....would complicate testing no?
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.
so you could do for example:
...
{{polaris-inline-error data-test-inline-error="email-error"}}
{{polaris-inline-error data-test-inline-error="password-error"}}
and just directly use these selectors in tests....
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.
ah got it 🙂👍🏻. May be we could add a note in our readme doc somewhere under the testing section about 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.
agreed...will add
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! Can you add a documentation file for the component though please?
|
||
init() { | ||
this._super(...arguments); | ||
assert('[polaris-inline-error] Missing required `fieldID` param!', this.get('fieldID')); |
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.
The React component doesn't seem to care about this... why do we?
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.
the docs mention it's required...I'm fine with dropping this, tbh
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.
True... don't see anything in the code enforcing it but maybe there's something in React that requires the props
to be present 🤷♂️
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.
TypeScript
}} | ||
`); | ||
|
||
assert.dom('[data-test-inline-error]').exists(); |
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.
These test selectors are pretty neat 😄
fix #175