The default settings value was looking for html_* and the default values are set with html5_*

I also change some of the defaults. I don't think modernizr should be enabled by default as it's not packages with the theme.

Patched attached.

CommentFileSizeAuthor
#2 settings.patch6.46 KBericduran
#1 settings.patch3.72 KBericduran

Comments

ericduran’s picture

ericduran commented
StatusFileSize
new settings.patch3.72 KB

uploading patch.

ericduran’s picture

ericduran commented
StatusFileSize
new settings.patch6.46 KB

Updated patch.

This patch adds a proper link to the Modernizr website and it also removes all the ":" from the t function as I don't think that should be included in the translation.

ericduran’s picture

ericduran commented

I just tought of a couple of things.
- We would probably want to add some kind of check when selecting to use Modernizr, and maybe more helpful instruction for setting it up correctly.
- If we're not packaging it up in the theme also think we should have it be included in Libraries and /sites/all/themes/html5_base/js/.
- Also I just notice the instruction are static and they shouldn't be, in some cases the themes might live in sites/defaults/themes or any number of directories. (this can be fixed by using libraries, or by just using a different set of instructions.)

ericduran’s picture

ericduran commented
Status: Needs review » Needs work

changing status after my post.

alanburke’s picture

alanburke commented

We could use hook_requirements to check for the js file,
if this were a module.

I already posted some drush integration in #915884: Drush Integration to download Modernizr js,
and that had to live in the HTML5 tools module.

ericduran’s picture

ericduran commented

hmmm, I can't really create a patch right now but we can implement the check on the checkbox using an element validate callback function for the use modernizr checkbox. Then it can error out if the use modernizr checkbox is check and the file isn't included, we can also provided a link in the error message.

mason@thecodingdesigner.com’s picture

mason@thecodingdesigner.com commented

@ericduran: I just committed these changes and a few more to the theme-settings.php and .info files. I'm curious to see what we can to to make modernizr work as a default setting. Not sure I follow how you outline it to work tho. Care to elaborate or write a patch?

mason@thecodingdesigner.com’s picture

mason@thecodingdesigner.com commented
Status: Needs work » Postponed (maintainer needs more info)
ericduran’s picture

ericduran commented
Status: Postponed (maintainer needs more info) » Fixed

@canaryMason, Yea, I was just rambling before lol. But I'll write a patch to validate if Modernizr is available before enabling the option.

But that's actually a separate issue and this issue can be consider fixed. :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.