Support html attributes in angle bracket components #293

mcfiredrill · 2019-11-19T00:41:56Z

Support for angle bracket components.
docs: #292

I know at the least ...attributes needs to be added to the template, I'm still working out if any other code can be removed or if anything else is needed.

mcfiredrill · 2019-11-19T00:44:26Z

addon/components/file-upload/template.hbs

@@ -7,6 +7,7 @@
  disabled={{disabled}}
  onchange={{action "change" value="target.files"}}
  hidden


Can these other attributes be removed in this template since they are now passed in as regular HTML attributes?

accept={{accept}} capture={{capture}} multiple={{multiple}} disabled={{disabled}}

Or perhaps they can just be given defaults in the template and then the ...attributes will override them?

This would be a breaking change as we won't support curly bracket invocation syntax anymore but I think that's fine as we prepare a new major anyways. @Alonski @gilest any thoughts on that one?

The current implementation is classic component classes which support the angle bracket invocation syntax.

Important to remember that these are separate concepts in ember:

component type/class (classic or glimmer)

component invocation (curly or angle)

I think that classic components are a more flexible implementation for now since it supports both new and older ember apps.

I just read the ember-angle-bracket-invocation-polyfill docs and it seems like the polyfill itself supports ...attritubes, but using them within our own component template may mean we no longer support curly invocation.

Maybe we could move ember-angle-bracket-invocation-polyfill from devDependencies to dependencies. That might give us a hybrid solution.

@gilest I fear you are missing inner-html vs outer-html in your concepts. Components using inner-html (@ember/component without tagName: '') do not play well together with ...attributes cause the HTML attributes passed on invocation would be set on the the element defined by component and on the elements that have ...attributes.

E.g. if you have a component <FormElement> like this:

export default Component.extend({ tagName: 'label', });

<input ...attributes>

And you invoke it with <FormElement required /> the rendered HTML would be:

<label required> <input required> </label>

That's not what you want in most cases.

You're right that's an important distinction! I think tagName: '' was kind of discouraged but I've used it a lot myself.

jelhan

@mcfiredrill Thanks for starting the work on this one!

As @knownasilya pointed out here #292 (comment) the components needs to be refactored to tagless components cause otherwise HTML attributes are applied to the tag defined by them as well.

jelhan · 2019-11-19T12:49:29Z

addon/components/file-upload/template.hbs

@@ -7,6 +7,7 @@
  disabled={{disabled}}
  onchange={{action "change" value="target.files"}}
  hidden


This would be a breaking change as we won't support curly bracket invocation syntax anymore but I think that's fine as we prepare a new major anyways. @Alonski @gilest any thoughts on that one?

mcfiredrill · 2019-11-22T00:49:53Z

As @knownasilya pointed out here #292 (comment) the components needs to be refactored to tagless components cause otherwise HTML attributes are applied to the tag defined by them as well.

Sounds good. Seems like I could move the label for file-upload directly into the template.

Will need to refactor away from using attributeBindings as well.

mcfiredrill · 2019-11-22T00:55:54Z

Sounds good. Seems like I could move the label for file-upload directly into the template.

Is it necessary for the label to wrap around the entire template, as in the current implementation?
If not, I have a feeling this check for VALID_TAGS could be removed.
https://github.com/adopted-ember-addons/ember-file-upload/blob/master/addon/components/file-upload/component.js#L160

jelhan · 2019-11-22T06:22:57Z

Is it necessary for the label to wrap around the entire template, as in the current implementation?
If not, I have a feeling this check for VALID_TAGS could be removed.
https://github.com/adopted-ember-addons/ember-file-upload/blob/master/addon/components/file-upload/component.js#L160

Oh, I wasn't aware of this. The main question seems to be: Should we support changing the tag of <label> at all? If so, that might be a show-stopper. If I didn't missed something that's not yet supported in template only components, is it?

I think we should ship valid HTML and drop that use case at all. Valid HTML would also be not wrapping input by label but assigning them to each user using for attribute on label.

@Alonski, @gilest Any thoughts?

mcfiredrill · 2019-11-22T08:22:26Z

FWIW this is already looking to be a breaking change.

I think we should ship valid HTML and drop that use case at all. Valid HTML would also be not wrapping input by label but assigning them to each user using for attribute on label.

👍 for valid HTML.

mcfiredrill · 2019-11-22T08:23:26Z

That said I think I don't 100% understand why the component was implemented as a label in the first place.

gilest · 2019-11-22T22:32:49Z

Yeah I think the <label> element that wraps the <input> is an interactive (clickable, and maybe draggable) area in many browsers.

We should make sure we follow the spec closely here for usability and accessibility.

gilest · 2019-11-22T22:34:43Z

@mcfiredrill Love the enthusiasm you're bringing 👍

But this is a pretty long-running addon, and I think supporting legacy apps and curly invocation is something we should keep doing for a while yet.

mcfiredrill · 2019-11-24T04:00:07Z

@gilest Thanks a lot for your feedback!

I haven't looked at it much yet so I'm going to investigate what benefits using ember-angle-bracket-invocation-polyfill in dependencies would bring.

I am thinking to maybe follow this advice, and not worry about supporting html attributes for now.
#292 (comment)

I may open a new PR or just totally redo this one.

gilest · 2019-11-24T23:19:03Z

@gilest Thanks a lot for your feedback!

I haven't looked at it much yet so I'm going to investigate what benefits using ember-angle-bracket-invocation-polyfill in dependencies would bring.

The polyfill is currently included in devDependencies so that we can test angle bracket invocation in our integration specs.

Moving it to dependencies would mean that users of the addon will get it too. This would allow us to use (while still supporting older versions of ember) angle bracket syntax within the addon's component templates (rather than just the tests).

I am thinking to maybe follow this advice, and not worry about supporting html attributes for now.
#292 (comment)

I may open a new PR or just totally redo this one.

Sounds like a good approach 👍

mcfiredrill · 2021-06-14T05:25:55Z

Hmm I'm confused, I see the link on github (actually a 404) saying the build failed.
But I found the build on travis just by clicking around there and it seemed to have passed:
https://travis-ci.org/github/adopted-ember-addons/ember-file-upload/builds/774490653

gilest · 2021-06-16T12:31:55Z

@mcfiredrill Please push again – we're on GitHub actions now 😅

mcfiredrill · 2021-06-17T01:15:10Z

@gilest Nice! Merged master.

mcfiredrill · 2021-06-17T09:31:43Z

Tests are passing but I'm trying to remember exactly what I was trying to do with this PR (nearly 3 years ago!), and it looks like I wanted to re-do at least a large portion of it.

I may open a new PR or just totally redo this one

gilest · 2021-09-25T00:04:30Z

Both FileDropzone and FileUpload implement splattributes since #514 which has been released as 4.1.0-beta.0

mcfiredrill added 2 commits November 17, 2019 22:34

add splattributes to file-upload template

d4e5453

fix typo

c78348f

mcfiredrill commented Nov 19, 2019

View reviewed changes

jelhan requested changes Nov 19, 2019

View reviewed changes

mcfiredrill changed the title ~~[WIP] Support angle brackets~~ [WIP] Support html attributes in angle bracket components Nov 28, 2019

jelhan mentioned this pull request Feb 27, 2020

Roadmap to v3 #306

Closed

6 tasks

gilest added breaking enhancement labels Feb 27, 2020

gilest mentioned this pull request Mar 12, 2020

Roadmap to v5 #331

Closed

22 tasks

jelhan mentioned this pull request Aug 25, 2020

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

Closed

mcfiredrill changed the title ~~[WIP] Support html attributes in angle bracket components~~ Support html attributes in angle bracket components Jun 14, 2021

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

0edc993

gilest mentioned this pull request Sep 3, 2021

Upgrade to glimmer components and native classes #514

Merged

gilest closed this Sep 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support html attributes in angle bracket components #293

Support html attributes in angle bracket components #293

mcfiredrill commented Nov 19, 2019 •

edited

Loading

mcfiredrill Nov 19, 2019

jelhan Nov 19, 2019

gilest Nov 22, 2019 •

edited

Loading

jelhan Nov 24, 2019

gilest Nov 24, 2019

jelhan left a comment

jelhan Nov 19, 2019

mcfiredrill commented Nov 22, 2019

mcfiredrill commented Nov 22, 2019

jelhan commented Nov 22, 2019

mcfiredrill commented Nov 22, 2019

mcfiredrill commented Nov 22, 2019

gilest commented Nov 22, 2019 •

edited

Loading

gilest commented Nov 22, 2019 •

edited

Loading

mcfiredrill commented Nov 24, 2019

gilest commented Nov 24, 2019 •

edited

Loading

mcfiredrill commented Jun 14, 2021

gilest commented Jun 16, 2021

mcfiredrill commented Jun 17, 2021

mcfiredrill commented Jun 17, 2021

gilest commented Sep 25, 2021

Support html attributes in angle bracket components #293

Support html attributes in angle bracket components #293

Conversation

mcfiredrill commented Nov 19, 2019 • edited Loading

mcfiredrill Nov 19, 2019

Choose a reason for hiding this comment

jelhan Nov 19, 2019

Choose a reason for hiding this comment

gilest Nov 22, 2019 • edited Loading

Choose a reason for hiding this comment

jelhan Nov 24, 2019

Choose a reason for hiding this comment

gilest Nov 24, 2019

Choose a reason for hiding this comment

jelhan left a comment

Choose a reason for hiding this comment

jelhan Nov 19, 2019

Choose a reason for hiding this comment

mcfiredrill commented Nov 22, 2019

mcfiredrill commented Nov 22, 2019

jelhan commented Nov 22, 2019

mcfiredrill commented Nov 22, 2019

mcfiredrill commented Nov 22, 2019

gilest commented Nov 22, 2019 • edited Loading

gilest commented Nov 22, 2019 • edited Loading

mcfiredrill commented Nov 24, 2019

gilest commented Nov 24, 2019 • edited Loading

mcfiredrill commented Jun 14, 2021

gilest commented Jun 16, 2021

mcfiredrill commented Jun 17, 2021

mcfiredrill commented Jun 17, 2021

gilest commented Sep 25, 2021

mcfiredrill commented Nov 19, 2019 •

edited

Loading

gilest Nov 22, 2019 •

edited

Loading

gilest commented Nov 22, 2019 •

edited

Loading

gilest commented Nov 22, 2019 •

edited

Loading

gilest commented Nov 24, 2019 •

edited

Loading