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

Autogenerate the customizer's variables form #11158

Merged
merged 3 commits into from Jan 9, 2014
Merged

Conversation

cvrebert
Copy link
Collaborator

Implements #11095 (or the main part of it anyway). This could use a lot of testing and review.
/cc @twbs/team

This doesn't address the issue of JS plugin or LESS file interdependencies, and it doesn't generate the checkbox sections at the top of the customizer page.

pretty: true,
data: function (dest, src) {
/*
Mini-language:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you guys wanna tweak the syntax, I'm all ears.

Choose a reason for hiding this comment

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

this whole piece of code is awesomeness! well done (Y)

@cvrebert
Copy link
Collaborator Author

After some testing, I'm now fairly confident about this from a technical standpoint, although the template definitely does need some styling love.
(In any event, I'd like to squash more of the commits before final merge though.)

@mdo
Copy link
Member

mdo commented Oct 21, 2013

Holy shit so much <3 for doing this. Will dive in after 3.0.1 ships. <3

return markdown.toHTML(markdownString.trim()).slice(3, -4);
}
function VarDocstring(markdownString) {
// the slice removes the <p>...</p> wrapper output by Markdown processor
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should move this comment into markdown2html()

@zlatanvasovic
Copy link
Contributor

@cvrebert This also removes unneeded Accordition section from Customizer.

@@ -178,7 +353,7 @@ module.exports = function(grunt) {
grunt.registerTask('validate-html', ['jekyll', 'validation']);

// Test task.
var testSubtasks = ['dist-css', 'jshint', 'qunit', 'validate-html'];
var testSubtasks = ['dist-css', 'jshint', 'qunit', 'build-customizer-vars-form', 'validate-html'];
Copy link
Contributor

Choose a reason for hiding this comment

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

build-customizer-vars is much better (and shorter) name for task than build-customizer-vars-form.

@zlatanvasovic
Copy link
Contributor

@cvrebert Could you rewrite these things to be just in Jekyll? Jekyll 1.3.0 datafiles now can replace Jade. ;)

@cvrebert
Copy link
Collaborator Author

cvrebert commented Nov 6, 2013

@zdroid Meh.

@zlatanvasovic
Copy link
Contributor

@cvrebert Or give me a chance to do that thing (just wait some time to GitHub update the Jekyll)? :D

@ssorallen
Copy link
Contributor

@zdroid I think the idea of this pull request was to generate the page from the .less files so the data stays in one place. Moving some of it to YAML data files would make it possible for them to get out of sync again.

Or what did you have in mind for the YAML data files?

@cvrebert
Copy link
Collaborator Author

cvrebert commented Nov 6, 2013

@ssorallen Well, you could generate YAML from the LESS, which Jekyll could then template into HTML. It'd be another level of indirection/complexity though.

@zlatanvasovic
Copy link
Contributor

I suggested Jekyll to avoid importing Jade (and maybe Markdown?).

2013/11/7 Chris Rebert notifications@github.com

@ssorallen https://github.com/ssorallen Well, you could generate YAML
from the LESS, which Jekyll could then template into HTML. It'd be another
level of indirection/complexity though.


Reply to this email directly or view it on GitHubhttps://github.com//pull/11158#issuecomment-27923751
.

Zlatan Vasović - ZDroid

@cvrebert
Copy link
Collaborator Author

@mdo Any feedback man?

@mdo
Copy link
Member

mdo commented Dec 8, 2013

This all looks glorious. There are a few things that came to mind:

  • Column sizing (I mentioned this in a code comment) needs to happen. A generic parent wrapper should suffice so we can make it into a grid column.
  • Need those for attributes on the labels.
  • v3 Customizer allow edit rather than overwrite #9829 asks for values instead of placeholders—thoughts?

Everything else is looking pretty good at this point. What are our options for expanding this language to include more granularity with subheading and such?

@mdo
Copy link
Member

mdo commented Dec 8, 2013

Also, I merged master and may have fucked something up 😁.

@zlatanvasovic
Copy link
Contributor

@mdo drop these commits :O

@zlatanvasovic
Copy link
Contributor

#9829 is good idea, should be done.

@cvrebert
Copy link
Collaborator Author

cvrebert commented Dec 9, 2013

@mdo Made the changes you requested. Also clobbered that merge with a proper rebase.

Everything else is looking pretty good at this point. What are our options for expanding this language to include more granularity with subheading and such?

Just tell me what syntax/scheme you desire, and I'll implement it 😄

@cvrebert
Copy link
Collaborator Author

  • TODO: add //=== for <h3> subsections, per mdo's request

@zlatanvasovic
Copy link
Contributor

@cvrebert Can you, after a completing pull request, merge these commits into one? Commit history looks so bad. :D

@cvrebert
Copy link
Collaborator Author

Yes, I definitely plan to do some squashing before the final merge.

@mdo
Copy link
Member

mdo commented Dec 12, 2013

As part of this, or a follow up, can you generate the list of Glyphicons on the Components page? Would be most useful for adding Flash-based click-to-copy.

@cvrebert
Copy link
Collaborator Author

@mdo Punted Glyphicons to #11862.

@zlatanvasovic
Copy link
Contributor

And auto-generate component list (I may do it, when autogenerating variables is finished)! :)

@mdo
Copy link
Member

mdo commented Dec 15, 2013

@cvrebert Cool, fine by me. Where do we stand on this then? I know I have to finish up the code comments revamp from the variables file, but anything else still open? Let's put together a quick check list if there's a few things.

@cvrebert
Copy link
Collaborator Author

I need to add support for another level of sectioning like you asked for, and I'd like to squash this PR into way fewer commits.

@@ -206,6 +206,182 @@ module.exports = function (grunt) {
docs: {}
},

jade: {
Copy link
Member

Choose a reason for hiding this comment

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

i feel like it's pretty gnarly this thing has to exist here… is it possible to make this script a node package, require it and execute it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, agreed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@cvrebert
Copy link
Collaborator Author

TODO:

  • move the parser into a separate file/module
  • once there's a /docs/, move the jade template & the parser JS files to be under it

@ghost ghost assigned cvrebert Jan 6, 2014
@cvrebert
Copy link
Collaborator Author

cvrebert commented Jan 8, 2014

@mdo Okay, this now supports the h3 syntax you requested. Can we merge this bad boy?

@zlatanvasovic
Copy link
Contributor

@cvrebert Did you merge master? It has /docs/ now. Also, squash these commits if possible. :)

@juthilo
Copy link
Collaborator

juthilo commented Jan 8, 2014

@zdroid Squashing commits would make the most sense once we know there's nothing left to be done, don't you think? :)

@zlatanvasovic
Copy link
Contributor

@juthilo Yup, that's why I said it as the last. :)

@cvrebert
Copy link
Collaborator Author

cvrebert commented Jan 8, 2014

It's already been squashed as much as I'm gonna squash it.

@zlatanvasovic
Copy link
Contributor

kk

2014/1/8 Chris Rebert notifications@github.com

It's already been squashed as much as I'm gonna squash it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/11158#issuecomment-31875876
.

Zlatan Vasović - ZDroid

@cvrebert
Copy link
Collaborator Author

cvrebert commented Jan 9, 2014

The variable docstrings might perhaps need further tweaking and more h3 subsections probably need to be added, but those are minor tweaks to the text rather than the parser code and can be done separately later.

cvrebert added a commit that referenced this pull request Jan 9, 2014
Autogenerate the customizer's variables form
@cvrebert cvrebert merged commit 9443044 into master Jan 9, 2014
@cvrebert cvrebert deleted the templated-customizer branch January 9, 2014 02:36
@mdo
Copy link
Member

mdo commented Jan 10, 2014

Thanks for getting this done @cvrebert <3. Everything looks great!

@mdo mdo mentioned this pull request Jan 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants