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

feat(components): make "template" method being optional. #545

Closed
wants to merge 1 commit into from
Closed

feat(components): make "template" method being optional. #545

wants to merge 1 commit into from

Conversation

stalniy
Copy link
Contributor

@stalniy stalniy commented Jan 24, 2016

Features for components:

  • made "template" method being optional
  • added ability to return DOM node/fragment from "template" method
  • added tests for component binding

CC: @Leeds-eBooks, @mikeric, @ansman

@stalniy
Copy link
Contributor Author

stalniy commented Jan 24, 2016

I did also implement support for dependent component and will pull request it as soon as this is closed (https://github.com/stalniy/rivets/tree/improved-component).

Also plan to add support for rv-content component which will insert existing component content to specified point (aka ng-content)

@benadamstyles
Copy link
Collaborator

As I mentioned in #500 I would like agreement from at least another contributor before merging in big changes, and this seems like a big change to me. Can you explain why the template method should be optional? I find coffeescript very hard to read...

@stalniy
Copy link
Contributor Author

stalniy commented Jan 25, 2016

@Leeds-eBooks Template method should be optional because sometimes you will want to proceed with existing template (e.g. tabset)

<tabset>
  <tab title="title #1">Title 1</tab>
</tabset>

or scrollbar component which implements custom scrollbar

PS: That's defenetly not a big change. The biggest amount of work was done on writing tests in JavaScript. So, I've added tests for some default behavior (in order to understand if everything is ok) and covered additional

Everything else is removal of code duplication and additional method renderTemplate.

EDIT: updated documentation. Currently documentation is a bit wrong as rivets.init ( https://github.com/mikeric/rivets/blob/master/src/rivets.coffee#L71) supports template functions which return HTMLElement and ComponentBinding does not (https://github.com/mikeric/rivets/blob/master/src/bindings.coffee#L228). This is due to code duplication which was removed in this request and also was added support for document fragments

@blikblum
Copy link
Contributor

IMO, the changes code wise is an improvement.

Regarding the ability of using the elements inside a component i think the way to go is to implement something similar to yield in Riot (http://riotjs.com/api/#yielding-nested-html) or slot in Vue ( http://vuejs.org/guide/components.html#Content_Distribution_with_Slots )

It's a very flexible feature covering the above case and others

@stalniy
Copy link
Contributor Author

stalniy commented Jan 25, 2016

@blikblum I've already done this.
Dependent components: https://github.com/stalniy/rivets/tree/improved-component

Transclusion: https://github.com/stalniy/rivets/tree/transclusion

Docs and tests are added as well so waiting for this to be merged and will proceed with other pull-requests

@stalniy
Copy link
Contributor Author

stalniy commented Jan 25, 2016

Also I wrote to @mikeric and asked him to provide access to repo for me and @blikblum (don't know your email so couldn't add you to CC)

@stalniy
Copy link
Contributor Author

stalniy commented Jan 26, 2016

For those who are interested in I plan currently maintain own fork of rivets. So, today I publish 1.0 version https://github.com/stalniy/rivets .

Did a lot of changes to components, everything is documented and tested. So, check spec/rivets and docs folders for more info

@Duder-onomy
Copy link
Collaborator

@stalniy I have been looking into this and it seems ok to me. You are adding features that people come to expect in libraries that have components. None of us can get ahold of @mikeric so, we will just work without him. Can you resolve the conflict in your branch? There were some test added for components in another PR. Yours are conflicting. I attempted to fix them myself but would prefer that you do it since you know your feature better than we do.

I have been looking through the docs on your fork. It looks like you are pushing things forward quite well. In order to get ALL the changes you have mentioned (transclusion etc) I would love to see more work done on the DOC's for components. There are many statements made that do not have corresponding example / explanation. For example, the statement : 'Controller is accessible inside template by component's name or by custom name specified in as option.' I see no example of this.

If you update your PR to resolve the conflict, I will comb through the docs and fix them where I can / notify you where I am unsure. Once we get the docs in line, I will merge this in.

We really appreciate your work. Please understand that components are one of the most confusing parts of Rivets right now (as evidenced by the amount of Issue's we get around them). If we can make components and their the docs better, then we can avoid some of that effort.

If @Leeds-eBooks and I had permissions to add more contributors. We probably would.

@stalniy
Copy link
Contributor Author

stalniy commented Feb 1, 2016

@Duder-onomy that is a great news! Will solve conflicts today

@stalniy
Copy link
Contributor Author

stalniy commented Feb 1, 2016

@Duder-onomy done. Will create new pull request as soon as this one is merged

@benadamstyles
Copy link
Collaborator

What about the es6 branch? All this work is going to need redoing, shouldn't we be putting new features into es6 and pushing forward with that?

@stalniy
Copy link
Contributor Author

stalniy commented Feb 1, 2016

@Leeds-eBooks I believe we don't have enough tests in order to switch to es6 branch

@stalniy
Copy link
Contributor Author

stalniy commented Feb 1, 2016

Also I believe there should be some convertor like coffeescript -> es6

Update: for example - http://stackoverflow.com/questions/28554713/any-automated-way-to-convert-coffeescript-to-es6

@stalniy
Copy link
Contributor Author

stalniy commented Feb 7, 2016

@Leeds-eBooks, @mikeric, @ansman, @Duder-onomy Are there any reasons that blocks the merge of this pull request?

All the changes here backward compatible. @Leeds-eBooks if you want me to create the same request for es6 branch just let me know. But I'd like to move components stuff forward.

@benadamstyles
Copy link
Collaborator

Sorry for the delay on this, @Duder-onomy was going to review and merge the components stuff you have done as I think he is more confident on this side of things. I'm sure he'll come to it as soon as he can!

…to return DOM node or fragment from "template" method
@stalniy
Copy link
Contributor Author

stalniy commented Feb 11, 2016

@Duder-onomy I know you are currently pretty busy but if you could just take a quick look at this and merge (as there are nothing you haven't seen) we would proceed with components enhancements

@stalniy
Copy link
Contributor Author

stalniy commented Feb 14, 2016

@Leeds-eBooks by the way the good example of the component with optional template is form. form component can extend default functionality of the form tag and provide some useful methods like form.isValid, form.errors, etc.

@stalniy stalniy closed this Mar 9, 2021
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.

4 participants