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

Proposed Syntax with Modifiers #342

Closed
cah-brian-gantzler opened this issue Dec 12, 2019 · 13 comments
Closed

Proposed Syntax with Modifiers #342

cah-brian-gantzler opened this issue Dec 12, 2019 · 13 comments

Comments

@cah-brian-gantzler
Copy link
Contributor

cah-brian-gantzler commented Dec 12, 2019

Modifier version with a more streamlined interface

<ol {{sortable-group onChange=this.update a11yAnnouncementConfig=this.myA11yConfig}}>
  {{#each model.items as |item|}}
    <li {{sortable-item model=item}}>
      {{item.name}}
      <span class="handle" {{sortable-handle}}>&varr;</span>
    </li>
  {{/each}}
</ol>

Modifiers will allow this addon to be used with other addons like ember-table, ember-yeti-table, ember-models-table

Modifiers are not meant for classic syntax, so to use these you would have to use angle-bracket syntax

Addons that would be used to do this:
ember-modifier-manager-polyfill > 2.12 . < 3.8
ember-modifier > 3.4 (Makes creating custom modifiers easier)

Optional
ember-native-class-polyfill (optional) > 3.4 < 3.6
ember-decorators-polyfill (optional) > 2.18 < 3.10

@cah-brian-gantzler
Copy link
Contributor Author

cah-brian-gantzler commented Dec 12, 2019

I found this emberjs/rfcs#112 (comment) (blast from the past). Anyone know if element-modifier helper made it into modifiers? That would be extremely helpful

@cah-brian-gantzler
Copy link
Contributor Author

element-modifier as a helper does not appear to exist, but I have it mostly working. Theres a lot of code I copied and pasted and feel I am sure I missed some. Hopefully tests will tell me where that it.

Husky threw me, wasnt aware of that but pretty cool. Going to try that out in my own repos

@cah-brian-gantzler
Copy link
Contributor Author

I have edited the purposed syntax based on a couple of things I found while implementing.

@cah-brian-gantzler
Copy link
Contributor Author

So there are a few things I dont like about this, specifically the yielded API and having to pass it around. But there is no modifier helper like there is for component, but been doing some thinking and I think I can actually pull off the below syntax which is pretty unbelievable. The a11y params would go on sortable-group as before.

<ol {{sortable-group onChange=this.update}}>
  {{#each model.items as |item|}}
    <li {{sortable-item model=item}}>
      {{item.name}}
      <span class="handle" {{sortable-handle}}>&varr;</span>
    </li>
  {{/each}}
</ol>

@cah-brian-gantzler
Copy link
Contributor Author

I have updated the initial proposed syntax for the new streamlined version. There is no longer a Sortable component at all. For each original component there is a corresponding modifier. So converting from one to the other is a very simple conversion.

@cah-brian-gantzler
Copy link
Contributor Author

Any possible movement on this?

@ygongdev
Copy link
Member

@cah-briangantzler sorry for the wait, I'll try to take a look at this later this week.
Let's drop anything lower than ember3.8 in our ember try testing, so the CI can properly reflect the test results.

@cah-brian-gantzler
Copy link
Contributor Author

Im not able to do that am I? Or are you asking me to change the ember-try config?

@ygongdev
Copy link
Member

Yes you are able to do that. I think you just need to remove the scenarios from the .travis.yml.

@chriskrycho
Copy link
Contributor

Was just thinking about this. I'm not sure I'll have time to evaluate any design proposal this week, unfortunately—don't consider yourselves blocked on me!

@ygongdev
Copy link
Member

I think the change overall is good. I don't think we need to worry about the failing build b/c it seems like there's some incompatibility with beta and canary which are on 3.17+, which is out of our current scope.
I've set aside some time this week to take a look. Apologize for the long delay!

@ygongdev
Copy link
Member

@cah-briangantzler I've released your changes as 2.2.0-beta.0. Give your awesome work a shot :)

@cah-brian-gantzler
Copy link
Contributor Author

Where we needed it we wrote it a different way :) Ill have to create a branch and see how much it will take to upgrade it.

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

No branches or pull requests

3 participants