glint icon indicating copy to clipboard operation
glint copied to clipboard

Allow attributes duplication on element

Open lifeart opened this issue 2 years ago • 3 comments

It will be great to have element attributes representation as [[name, value]] array, instead of Record<name, value> in applyAttributes function.

It allow us to have fine-granted control on attributes behaviour and not eagerly fail with An object literal cannot have multiple properties with the same name TS error.

Why it may be needed?

In glimmer-next we allow users to have multiple class attributes to avoid ugly concat statements.

<div 
  class="base-class"
  class={{controlClass}}
  class={{behaviourClass}}
>
</div>

Sample: https://github.com/lifeart/glimmer-next/blob/6b90ffce23ed26e68b2d8ecbc3bbd3d892e2195e/src/components/pages/benchmark/Row.gts#L87

It bring clear separation of concerns (static class parts, dynamic class parts).

lifeart avatar Jan 22 '24 17:01 lifeart

Why not?:

<div class="base-class {{controlClass} {{behaviorClass}}">
</div>

semantically, (like, for other attributes), setting multiple times overrides other values.

For example, this JSBin: https://jsbin.com/leyizogeli/edit?html,output

NullVoxPopuli avatar Jan 22 '24 20:01 NullVoxPopuli

At the moment there is no way to allow ...attributes to drop some classes (because it's merged by default) But with proposed approach we may allow constructions like this:

<div class="non-overridable" ...attributes class="im-removed-if-attributes-has-class-property">

semantically, (like, for other attributes), setting multiple times overrides other values.

-Yes, but semanically classes are special, because allow to have multiple values, divided by spaces. We could support all values here, but allow only class duplication in template-lint.

Why not?:

<div class="base-class {{controlClass} {{behaviorClass}}">
</div>

Because it's really easy to loose control in readability and have something like:

<div class="base-class {{or @isActive controlClass}} {{if this.isTrue behaviorClass " col-sm-12"}}">

<div 
  class="base-class"
  class={{or @isActive controlClass}}
  class={{if this.isTrue behaviorClass  "col-sm-12"}}
>

lifeart avatar Jan 22 '24 20:01 lifeart

@NullVoxPopuli it’s not an Ember proposal, it’s just about flexibility of internal architecture, since in hbs ast we already have attributes represented as array of properties, not key-value pojo.

We not getting DX worse, because template-lint has rule to warn about duplicated attributes: https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-duplicate-attributes.md

lifeart avatar Jan 23 '24 03:01 lifeart