Misc updates for v6 #620
Misc updates for v6 #620
Conversation
There was a problem hiding this comment.
Big ol' PR to trawl through, I've left some comments on a couple of things I noticed. I think we need to test PolarisTextField carefully around events (focus/blur etc.), and multiline (re)sizing etc. based on what I've seen in the code. Didn't pay a huge amount of attention to the test file updates, they're just compacting some multi-line things down to single line and changing to angle bracket notation right?
| <div | ||
| class="Polaris-TextField__DummyInput" | ||
| {{did-insert this.this.setContentNode}} | ||
| > | ||
| {{this.finalContents}} | ||
| </div> | ||
|
|
||
| {{#if @minimumLines}} | ||
| <div | ||
| class="Polaris-TextField__DummyInput" | ||
| {{did-insert this.setMinimumLinesNode}} | ||
| > | ||
| {{this.contentsForMinimumLines}} | ||
| </div> |
There was a problem hiding this comment.
Have you checked that multiline text fields render/resize to fit their content correctly with these changes? There's a comment at the top of this file that says these two dummy inputs need to be on a single line to render the correct size. If that's no longer the case, can we remove the comment?
There was a problem hiding this comment.
checked now, fixed some issues 🙌
| > | ||
| <div | ||
| class="Polaris-TextField__DummyInput" | ||
| {{did-insert this.this.setContentNode}} |
There was a problem hiding this comment.
Extra this. in here 😬
| {{did-insert this.this.setContentNode}} | |
| {{did-insert this.setContentNode}} |
| @@ -1,4 +1,4 @@ | |||
| {{#let (element (or @htmlTag "p")) as |DisplayText|}} | |||
| {{#let (element (or (or @htmlTag @tagName) "p")) as |DisplayText|}} | |||
There was a problem hiding this comment.
Can just be
| {{#let (element (or (or @htmlTag @tagName) "p")) as |DisplayText|}} | |
| {{#let (element (or @htmlTag @tagName "p")) as |DisplayText|}} |
instead of nesting ors? Same applies in a few other places where you've added htmlTag support.
| spellcheck={{@spellCheck}} | ||
| value={{this.normalizedValue}} | ||
| class={{this.inputClassName}} | ||
| aria-multiline={{@multiline}} |
There was a problem hiding this comment.
Does this add aria-multiline="false" or anything similar if @multiline={{true}} is passed to the PolarisTextField? If so, can we check if that replicates the React implementation and if not, update it? 🙇
There was a problem hiding this comment.
made sure it matches react implementation
| > | ||
| <div | ||
| class="Polaris-TextField__DummyInput" | ||
| {{did-insert this.this.setContentNode}} |
| <div | ||
| class="Polaris-TextField__DummyInput" | ||
| {{did-insert this.this.setContentNode}} | ||
| > | ||
| {{this.finalContents}} | ||
| </div> | ||
|
|
||
| {{#if @minimumLines}} | ||
| <div | ||
| class="Polaris-TextField__DummyInput" | ||
| {{did-insert this.setMinimumLinesNode}} | ||
| > | ||
| {{this.contentsForMinimumLines}} | ||
| </div> |
There was a problem hiding this comment.
checked now, fixed some issues 🙌
| spellcheck={{@spellCheck}} | ||
| value={{this.normalizedValue}} | ||
| class={{this.inputClassName}} | ||
| aria-multiline={{@multiline}} |
There was a problem hiding this comment.
made sure it matches react implementation
text-fieldcomponent during modernization PR@tagNameto some components and replaces it withhtmlTag.getwhere it's not neededv6so we'll support those through tov7labelComponentforchoicecomponent