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

Simpler API #25

Closed
wants to merge 34 commits into from
Closed

Simpler API #25

wants to merge 34 commits into from

Conversation

guscost
Copy link

@guscost guscost commented Jun 18, 2017

This implements changes to the API according to discussions in #12, #20 and #23:

  • Merged $cell and $type keywords, now there is only a $cell keyword containing the tag name.
  • Renamed $components to be called $contents since it's a bit shorter and possibly clearer.
  • Changed convention for custom variables, now they are prefixed with a $ along with keywords.
  • Added an imperative API. Run Cell() passing in a cell object to start up after page load.
  • Additionally assigned the API to a global Cell variable ;)
  • Finally, removed waiting for the page load event before running the first create(), since developers can always initialize a cell later if our objects are created later. This is my preferred behavior, but it means the cell script needs to go at the end of the page for the automatically-loaded demo.

Here's a gist demonstrating the new API (the flux-like stuff is just another experiment):

https://gist.github.com/guscost/d47e3437e75cd72293cede3ef847121d

And a live demo:

https://guscost.github.io/cell/test/todos.html

@guscost guscost mentioned this pull request Jun 18, 2017
@devsnek
Copy link
Contributor

devsnek commented Jun 18, 2017

$contents isn't inherently explanatory of the fact that it will include elements of the same type as the parent, as $children (which is a general standard in programming) or $components (which is also kinda a general standard) would be.

Creating an API is cool, not sure about polluting global just yet. Perhaps it could be scoped into the root cells instead of polluting global, although that falls back to letting cell run for you.

@guscost
Copy link
Author

guscost commented Jun 18, 2017

How about a UMD module? I've gotta test this but the header now loads as AMD, CommonJS or on the root as a fallback.

@guscost
Copy link
Author

guscost commented Jun 18, 2017

OK, the UMD module looks good. Also fixed merge conflicts.

@gliechtenstein
Copy link
Contributor

Wow thanks for the PR, I really appreciate that you've questioned a lot of things.

First of all, there's a lot of things going on here, which is why I've been hesitating where to start.

Since this PR is related to pretty much the majority of all the currently open issues, I've been trying to figure out a best way to respond most efficiently.

I think it would be best if I share my side of the story as a single post (how I came up with the naming convention, architecture, and little implementation details, and why) and then take it from there instead of replying to each thread, so that we can centralize the discussion around these issues.

I'm going to write up something today and post it as github issue sometime today, I hope to continue the conversation about this PR after that. Thank you!

@jordalgo
Copy link

👍 Overall, great changes, especially:

Added an imperative API. Run Cell() passing in a cell object to start up after page load.

@guscost
Copy link
Author

guscost commented Jun 19, 2017

@gliechtenstein sounds good, I'll try to keep these somewhat in sync with other updates but this particular branch could just be a vehicle for discussion too.

@guscost guscost closed this Jun 20, 2017
@piranna
Copy link

piranna commented Jun 25, 2017

Cell variable

It's not a variable, it's a namespace ;-) They are similar but have subtle differences in concept :-)

@guscost
Copy link
Author

guscost commented Jun 25, 2017

It's both, depending on the context.

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.

5 participants