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

Adds a :properties directive #68

Closed
wants to merge 2 commits into from

Conversation

zamith
Copy link
Contributor

@zamith zamith commented Apr 16, 2020

Why:

  • This allows sending props down to the underlying components without
    having to redefine them all. This is just an experience on how it
    could work for html tags

Related to #2

This change addresses the need by:

  • Adding a :properties directive that for a component will take a map and
    add it to the props in the assigns as __dynamic_props__. If it is a tag,
    it will go over those __dynamic_props__ and add them as attributes.

Notes:

  • Could not use :props as it's already used by slots.
  • On the html tag don't really need the value of the directive, but it's
    not registered as a directive otherwise. Might look into that later.
  • Not a fan of relying on the position in the lists, but couldn't figure out a better way, tbh.

Why:

* This allows sending props down to the underlying components without
  having to redefine them all. This is just an experience on how it
  could work for html tags

This change addresses the need by:

* Adding a :props directive that for a component will take a map and add
  it to the props in the assigns as __dynamic_props__. If it is a tag,
  it will go over those __dynamic_props and add them as attributes.
* Slot already uses :props
* On the html tag don't really need the value of the directive, but it's
  not registered as a directive otherwise. Might look into that later.
@msaraiva
Copy link
Member

@zamith thanks a lot for the PR!

I took I look at the code and also tried a couple of alternatives to simplify the implementation, however, my feeling is that the current way to handle directives is far from ideal. I'll put this PR on hold until I finish #15. It should become much easier to properly implement this feature after that.

@zamith
Copy link
Contributor Author

zamith commented Apr 17, 2020

Yes, that would help a lot of the implementation, I think.

In terms of the proposal for how to use it though, does it make sense or do you have other ideas? Also not a fan of not being able to call it :props :/

@msaraiva
Copy link
Member

In terms of the proposal for how to use it though, does it make sense or do you have other ideas?

I think the behaviour should be very similar. The one thing we need to decide is how the values should be overridden. For instance, if we have:

<Component prop1="value1" :props={{ @some_props }} prop2="value2"/>

and the value of @some_props is [value1: "VALUE1", value2: "VALUE2", value3: "VALUE3"].

What should be the behaviour? We have 3 options:

  1. Dynamic overrides all static:
    <div prop1="VALUE1" prop2="VALUE2" prop3="VALUE3"/>

  2. Static overrides dynamic based on the order of definition:
    <div prop1="VALUE1" prop2="value2" prop3="VALUE3"/>

  3. Static always overrides dynamic, regardless the order:
    <div prop1="value1" prop2="value2" prop3="VALUE3"/>

I believe the less confusing would be #3.

Also not a fan of not being able to call it :props :/

I think we should call it :props. According to the specification, the slot element does not accept any extra attribute, just name. There are even some implementation of slots that use the attributes themselves as static slot props, e.g. <slot value1={{@value1}} value2={{@value2}}> instead of <slot :props={{ value1: @value1, value2: @value1 }} >. The rationale is that the <slot> is just proxy to the "real" content, so it makes sense to pass properties using the same abstraction, in our case, using :props.

@zamith
Copy link
Contributor Author

zamith commented Apr 17, 2020

Yup. Agree that #3 is what I would expect.

What I meant is that I tried calling it :props and it failed some test. I'm sure this is related to something on the translator I didn't quite get.

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 this pull request may close these issues.

2 participants