Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow attributes duplication on element #693

Open
lifeart opened this issue Jan 22, 2024 · 3 comments · May be fixed by #695
Open

Allow attributes duplication on element #693

lifeart opened this issue Jan 22, 2024 · 3 comments · May be fixed by #695

Comments

@lifeart
Copy link

lifeart commented Jan 22, 2024

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).

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Jan 22, 2024

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

@lifeart
Copy link
Author

lifeart commented Jan 22, 2024

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
Copy link
Author

lifeart commented Jan 23, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants