Update component docs to use angle bracket syntax #292

mcfiredrill · 2019-11-15T09:42:45Z

I tried use ember-angle-brackets-codemod, but it didn't seem to work on
.js files. So I edited the doc comments manually.

I tried use ember-angle-brackets-codemod, but it didn't seem to work on .js files. So I edited the doc comments manually.

jelhan

Thanks a lot for updating. Seems we missed it on the last PRs.

I think you need to rebase after #291 has been merged.

I fear we missed some work to fully support angle bracket invocation. I see two ways to address it:

Do not pass HTML attributes but arguments on component invocation or
fix missing attributes forwarding before merging this one.

jelhan · 2019-11-15T10:11:43Z

addon/components/file-dropzone/component.js

-                      onfileadd=(action "uploadImage")}}
+        <FileUpload name="photos"
+                    accept="image/*"
+                    multiple=true


multiple=true is not valid HTML, is it? I think it should be <FileUpload multiple />.

jelhan · 2019-11-15T10:12:41Z

addon/components/file-dropzone/component.js

-                      multiple=true
-                      onfileadd=(action "uploadImage")}}
+        <FileUpload name="photos"
+                    accept="image/*"


I fear we don't support passing HTML attributes to <FileUpload> yet. It doesn't seem to pass them to <input>. At least I don't see a ...attributes in the template.

...attributes are applied to the parent element unless tagName is set to a empty string.

jelhan · 2019-11-15T10:20:11Z

addon/components/file-dropzone/component.js

+        <FileUpload name="photos"
+                    accept="image/*"
+                    multiple=true
+                    @onfileadd={{action "uploadImage"}}>


The coding style looks different than what I would expect. I'm used to this one:

<FileUpload @onfileadd={{action "uploadImage"}} name="photos" accept="image/*" />

So it's basically about this rules:

If there are multiple lines, each attribute or property should be on a new line

It should be intended by two spaces.

Arguments should be before HTML attributes.

I thought that's also what's used in Ember docs but noticed that Ember docs aren't consistent on these ones.

I usually do html attributes first and then args, usually actions last

Hmm I wonder if there is any ember-template-lint rule about this...

@mcfiredrill I am glad you asked 😄
ember-template-lint/ember-template-lint#871
I requested this a while ago :)
ember-template-lint/ember-template-lint#684

Although it isn't exactly what @knownasilya mentioned

mcfiredrill · 2019-11-16T01:32:50Z

I fear we missed some work to fully support angle bracket invocation. I see two ways to address it:

Do not pass HTML attributes but arguments on component invocation or
fix missing attributes forwarding before merging this one.

Sounds good, I'm using this addon with angle brackets in my app now so I will make another PR with those fixes before getting this merged.

addon/components/file-dropzone/component.js

gilest · 2019-11-22T22:37:05Z

See file-upload-test.js for the angle-bracket API that is currently covered by integration tests.

It's not an absolutely ideal API (no html attribute support) but it does allow us to support a wide range of ember versions.

Alonski · 2019-11-23T18:25:37Z

@Turbo87 gave me a few tips on this:

I would prefix everything with @
if you don't then the fact that the ...attributes element is the <input type="file"> becomes public API and I'm not sure you'll want that in this case
I generally prefer a defined public API instead of the "pass everything to some child element"
...attributes should still be on the top-level element though so that CSS classes can be applied from the outside, to e.g. add margins on the component

Hmm so we should explicitly use @ and then inside the component use <input name={{@name}}> or something?

yeah, I think so

mcfiredrill · 2019-11-24T03:51:52Z

@Alonski Very good points! I didn't think about the basically becoming public API.

It seems like I got really hung up on getting the component to work with normal HTML attributes without really thinking about the tradeoffs or alternatives.

#292 (review)

I fear we missed some work to fully support angle bracket invocation. I see two ways to address it:

1 . Do not pass HTML attributes but arguments on component invocation or
2. fix missing attributes forwarding before merging this one.

Maybe @jelhan 's initial suggestion 1 here was a better idea after all. 🤔

mcfiredrill · 2019-11-24T03:52:29Z

@gilest Thank you, I didn't even realize that was in the tests! 😅

jelhan · 2019-11-24T10:13:17Z

@Alonski I'm not sure if I agree. Designing this component from scratch using an outer-html approach, would we really end up with this as best API?

<label ...attributes>
  <input>
</label>

I've the feeling that we would have done something like:

{{#if (has-block)}}
  {{yield
    (hash
      fileChanged=(action "change")
    )
  }}
{{else}}
  <label
    for={{inputId}}
  >
  <input
    id={{inputId}}
    type="file"
    onchange={{action "change"  value="target.files"}}
   ...attributes
  >
{{(/if}}

Turbo87 · 2019-11-24T11:07:45Z

@jelhan if you implement it that way you would have no way of controlling the margins around the component from the outside without adding another unnecessary wrapper component. I would consider the fact that the component is using a native input element inside an implementation detail that should not be relied upon.

mcfiredrill · 2019-11-28T16:48:21Z

Hmm so we should explicitly use @ and then inside the component use <input name={{@name}}> or something?

Using @ for all arguments seems to be supported already according to this test:
https://github.com/adopted-ember-addons/ember-file-upload/blob/master/tests/integration/components/file-upload-test.js

So I think if I just change all the arguments in the docs to be @ prefixed, would this PR be merge-able?

<input name={{@name}}>

I think we could leave the templates as is without @ prefixing the arguments in the template. This will keep them backwards compability with {{}} curly components, if I understand correctly?

I renamed my related PR to "Support html attributes in angle bracket components", since angle bracket components technically already work.

mcfiredrill · 2019-11-29T03:58:11Z

So I think if I just change all the arguments in the docs to be @ prefixed, would this PR be merge-able?

Done in aa98e71

Hmm build is failing...I haven't changed any code in this PR however. Someone want to try running it again for me? 🛠

mcfiredrill · 2019-12-21T07:24:56Z

bump...anyone know why my build failed? :)

Alonski · 2019-12-21T08:41:22Z

@mcfiredrill Restarted. Ping me on Discord if it fails again please 🙂

gilest · 2021-06-12T00:32:29Z

@mcfiredrill 👋 Hello from the future. Care to rebase this so the CI can run again?

mcfiredrill · 2021-06-14T04:36:38Z

Hi there, it's been awhile! I just merged master into this branch.

gilest

LGTM.

@jelhan Your requested changes had been addressed – yes?

gilest

Since (#476) onfileadd has been deprecated in favour of onFileAdd.

Also found a > was missing.

Also suggest we:

Drop action helper from examples
Use fn helper to pass arguments to callbacks

Please rebase 😅

addon/components/file-dropzone/component.js

addon/components/file-upload/component.js

Co-authored-by: Giles Thompson <[email protected]>

…-file-upload into angle-brackets-codemod

mcfiredrill · 2021-07-03T13:14:55Z

Sorry for the delay, I have merged master.

Also suggest we:

Drop action helper from examples
Use fn helper to pass arguments to callbacks

I could try to address these here, or that could be another PR?

gilest · 2021-07-11T22:55:27Z

Drop action helper from examples
Use fn helper to pass arguments to callbacks

I could try to address these here, or that could be another PR?

Looks like you addressed these in 71d3298

gilest

LGTM. Believe this syntax is correct for the current version.

gilest · 2021-07-11T22:56:47Z

@jelhan I think this can be merged now. Let me know if you still need changes

update docs to use angle bracket syntax

ab45892

I tried use ember-angle-brackets-codemod, but it didn't seem to work on .js files. So I edited the doc comments manually.

jelhan requested changes Nov 15, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into angle-brackets-codemod

8070f0d

mcfiredrill commented Nov 17, 2019

View reviewed changes

addon/components/file-dropzone/component.js Outdated Show resolved Hide resolved

Update addon/components/file-dropzone/component.js

0027111

mcfiredrill mentioned this pull request Nov 19, 2019

Support html attributes in angle bracket components #293

Closed

prefix @ arguments in angle bracket component examples

aa98e71

jelhan mentioned this pull request Aug 25, 2020

[Feature] Consume id and expose input focus state #388

Closed

Merge remote-tracking branch 'origin/master' into angle-brackets-codemod

861b77c

gilest self-requested a review June 15, 2021 04:35

gilest approved these changes Jun 15, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into angle-brackets-codemod

9f2d71d

gilest requested changes Jun 20, 2021

View reviewed changes

addon/components/file-dropzone/component.js Outdated Show resolved Hide resolved

addon/components/file-upload/component.js Outdated Show resolved Hide resolved

mcfiredrill and others added 2 commits June 22, 2021 08:40

Update addon/components/file-dropzone/component.js

56c59a4

Co-authored-by: Giles Thompson <[email protected]>

Update addon/components/file-upload/component.js

9b6a103

Co-authored-by: Giles Thompson <[email protected]>

gilest changed the title ~~update docs to use angle bracket syntax~~ Update component docs to use angle bracket syntax Jun 22, 2021

gilest added the documentation label Jun 22, 2021

mcfiredrill added 2 commits July 3, 2021 22:12

Merge branch 'angle-brackets-codemod' of github.com:mcfiredrill/ember…

71d3298

…-file-upload into angle-brackets-codemod

Merge remote-tracking branch 'origin/master' into angle-brackets-codemod

176c29d

gilest approved these changes Jul 11, 2021

View reviewed changes

gilest merged commit 1c1f146 into adopted-ember-addons:master Aug 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update component docs to use angle bracket syntax #292

Update component docs to use angle bracket syntax #292

mcfiredrill commented Nov 15, 2019

jelhan left a comment

jelhan Nov 15, 2019

jelhan Nov 15, 2019

knownasilya Nov 19, 2019

jelhan Nov 15, 2019

knownasilya Nov 15, 2019

mcfiredrill Nov 16, 2019

Alonski Nov 23, 2019

mcfiredrill commented Nov 16, 2019

gilest commented Nov 22, 2019 •

edited

Loading

Alonski commented Nov 23, 2019

mcfiredrill commented Nov 24, 2019

mcfiredrill commented Nov 24, 2019

jelhan commented Nov 24, 2019

Turbo87 commented Nov 24, 2019

mcfiredrill commented Nov 28, 2019 •

edited

Loading

mcfiredrill commented Nov 29, 2019

mcfiredrill commented Dec 21, 2019

Alonski commented Dec 21, 2019

gilest commented Jun 12, 2021 •

edited

Loading

mcfiredrill commented Jun 14, 2021

gilest left a comment

gilest left a comment •

edited

Loading

mcfiredrill commented Jul 3, 2021

gilest commented Jul 11, 2021

gilest left a comment

gilest commented Jul 11, 2021 •

edited

Loading

Update component docs to use angle bracket syntax #292

Update component docs to use angle bracket syntax #292

Conversation

mcfiredrill commented Nov 15, 2019

jelhan left a comment

Choose a reason for hiding this comment

jelhan Nov 15, 2019

Choose a reason for hiding this comment

jelhan Nov 15, 2019

Choose a reason for hiding this comment

knownasilya Nov 19, 2019

Choose a reason for hiding this comment

jelhan Nov 15, 2019

Choose a reason for hiding this comment

knownasilya Nov 15, 2019

Choose a reason for hiding this comment

mcfiredrill Nov 16, 2019

Choose a reason for hiding this comment

Alonski Nov 23, 2019

Choose a reason for hiding this comment

mcfiredrill commented Nov 16, 2019

gilest commented Nov 22, 2019 • edited Loading

Alonski commented Nov 23, 2019

mcfiredrill commented Nov 24, 2019

mcfiredrill commented Nov 24, 2019

jelhan commented Nov 24, 2019

Turbo87 commented Nov 24, 2019

mcfiredrill commented Nov 28, 2019 • edited Loading

mcfiredrill commented Nov 29, 2019

mcfiredrill commented Dec 21, 2019

Alonski commented Dec 21, 2019

gilest commented Jun 12, 2021 • edited Loading

mcfiredrill commented Jun 14, 2021

gilest left a comment

Choose a reason for hiding this comment

gilest left a comment • edited Loading

Choose a reason for hiding this comment

mcfiredrill commented Jul 3, 2021

gilest commented Jul 11, 2021

gilest left a comment

Choose a reason for hiding this comment

gilest commented Jul 11, 2021 • edited Loading

gilest commented Nov 22, 2019 •

edited

Loading

mcfiredrill commented Nov 28, 2019 •

edited

Loading

gilest commented Jun 12, 2021 •

edited

Loading

gilest left a comment •

edited

Loading

gilest commented Jul 11, 2021 •

edited

Loading