ember-bootstrap / ember-bootstrap

Ember-cli addon for using Bootstrap as native Ember components.
https://www.ember-bootstrap.com
Other
491 stars 195 forks source link

Unable to set custom element-id on form elements #1091

Closed basz closed 4 years ago

basz commented 4 years ago

In v4 setting @id="my-id" on a form element does not work as expected.

in v3 you could do

<form.element @id="password"
                @controlType="password"
                @property="password"
                @label={{t "model.identity.password"}}  as |element|>
    <element.control autocomplete="current-password"/>
  </form.element>

In v4 you need to add id="..." to the element.control

<form.element @_elementId="password"
                @controlType="password"
                @property="password"
                @label={{t "model.identity.password"}} as |element|>
    <element.control id={{element.id}} autocomplete="current-password"/>
  </form.element>

(the element will get id='password-field', label will also get for='password-field')

However @id="my-id" does not work anymore. @elementId="my-id" will throw an error (cause tagLess component), so, for now, we will need to do @_elementId="my-id".

jelhan commented 4 years ago

Why do you need to set a specific ID for the <form.element>?

basz commented 4 years ago

I use it you select by predictable id...

in cypress tests but also to focus or select text in application code.

jelhan commented 4 years ago

I don't think we consider this IDs as part of our public API. I would recommend to use data attributes as test selectors instead. As an alternative you can set an ID on the .form-group element using angle bracket syntax: <form.element id="foo" /> In both cases the IDs for control element and label would not be derived from it. But it should be straightforward to select these child elements of .form-group.

E.g. I'm often using a pattern like this:

<BsForm @model={{this.post}} data-test-form-for="post" as |form|>
  <form.element
    @label="Title"
    @property="title"
    data-test-form-element-for="title"
  />
  <form.element
    @controlType="textarea"
    @label="Text"
    @property="body"
    data-test-form-element-for="body"
  />
</BsForm>

The tests for this using Ember's default test helpers (including qunit-dom) would look like this:

await fillIn('[data-test-form-element-for="title"] input', 'Awesome blog post');
assert.dom('[data-test-form-element-for="title"] input').hasValue('Awesome blog post');
basz commented 4 years ago

Okay I have no issue with this... Just a difference from v3, so people know... thanks!

simonihmig commented 4 years ago

Yeah, we deprecated (in v3) and removed (in v4) arguments that just map to HTML attributes. @id and @class fall into that category as well, although we didn't have to support them explicitly (attributeBindings), as they were handled by Ember.Component automatically.

@jelhan we should probably mentioned that explicitly, at least in the Changelog!? Adding deprecations/warning seems unfavorable in terms of effort/benefit, as that would mean adding those to each and every component (root or child/contextual).

jelhan commented 4 years ago

@jelhan we should probably mentioned that explicitly, at least in the Changelog!? Adding deprecations/warning seems unfavorable in terms of effort/benefit, as that would mean adding those to each and every component (root or child/contextual).

Agree.