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

Failing test for concatenated classes on contextual glimmer components #16933

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Sep 1, 2018

We come from here: rwjblue/sparkles-component#7

Seems that in contextual components we can't add default classes to html elements that also receive another class on splattributes.

I don't even know where to look, but if someone can point me in the right direction I can try.

The issue can also be seen in this repo I created to play with sparkles-components: https://github.com/cibernox/sparkles-ember-basic-dropdown

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! We should add tests for contextual components with other attributes passed also since class has special behavior to merge, but we also should confirm that “normal” attributes work in the test suite.

@cibernox cibernox force-pushed the bug-concatenated-classes-on-contextual-glimmer-components-with-splattributes branch from ade0530 to d692103 Compare September 2, 2018 17:05
@cibernox
Copy link
Contributor Author

cibernox commented Sep 2, 2018

@rwjblue I added another test for non-concatenable attributes like title.

There is however one scenario where I don't know what the expected behavior is:

{{!-- ui-button.hbs --}}
<button title="foo" ...attributes></button>
{{!-- ui-form.hbs --}}
{{yield (hash button=(component "ui-button" title="bar"))}}
{{!-- application.hbs --}}
<UiForm as |f|>
  <f.button title="baz"></button>
</UiForm>

Will the title passes to the component helper be considered an attribute or an argument? I think the latter, so it shouldn't be merged used unless @title is used in the template. Is there a way to pass attributes to contextual components? Maybe we should consider it.

@rwjblue
Copy link
Member

rwjblue commented Sep 3, 2018

Correct, anything inside curlies (which the yielded contextual component is) is going to be passed as an argument (and not an attribute to be merged/splatted).

@cibernox
Copy link
Contributor Author

cibernox commented Sep 3, 2018

Any direction on where the problem may be? I free have time this week.

cibernox and others added 2 commits September 5, 2018 21:09
Fixes issue with attribute merging with dynamic angle bracket component
invocation (invoking paths, named args, locals, etc).
@rwjblue rwjblue force-pushed the bug-concatenated-classes-on-contextual-glimmer-components-with-splattributes branch from d692103 to 807c489 Compare September 6, 2018 01:22
@rwjblue
Copy link
Member

rwjblue commented Sep 6, 2018

Just pushed the fix for these (after backporting it in glimmer-vm).

@rwjblue rwjblue merged commit bc505f5 into emberjs:master Sep 6, 2018
@rwjblue
Copy link
Member

rwjblue commented Sep 6, 2018

Thank you @cibernox!

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.

None yet

2 participants