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

Static dependencies have been removed and replaced by components #20

Merged
merged 5 commits into from
Sep 23, 2013
Merged

Static dependencies have been removed and replaced by components #20

merged 5 commits into from
Sep 23, 2013

Conversation

Gta-Cool
Copy link
Contributor

Hi, I removed the static dependencies in favor of components packages required from composer. I hope you will appreciate my changes.

@@ -21,7 +21,8 @@
],
"require": {
"php": ">=5.3.2",
"symfony/framework-bundle": ">=2.0,<3.0-dev"
"symfony/framework-bundle": ">=2.0,<3.0-dev",
"components/html5-boilerplate": "*"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should point to version 4.3 or better to 4.3.* here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's impossible because the component has only one master branch without tag.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You right! I proceed to change.

@lmammino
Copy link
Member

Great contribution @Gta-Cool,
I'm very happy with it. Thanks a lot!
I suppose this one should resolve what has been asked on #17 and simplify the upgrade process to reflect the changes next h5bp releases will introduce.

I've added some comments on your code, please take two minutes to review them before I proceed with the merge.

I noticed you marked this version as 4.3 (as the latest h5bp version). As OryzoneBoilerplateBundle versions tends to follows strictly the h5bp versions, I would like to ask you if you fully upgraded the twig template to fully reflect the new changes introduced by the new h5bp version, particoularly:

(https://github.com/h5bp/html5-boilerplate/blob/v4.3.0/CHANGELOG.md)

@@ -29,7 +30,7 @@
"target-dir": "Oryzone/Bundle/BoilerplateBundle",
"extra": {
"branch-alias": {
"dev-master": "4.2.x-dev"
"dev-master": "4.3.x-dev"
Copy link
Member

Choose a reason for hiding this comment

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

Do you fully upgraded the twig template to reflect the new changes introduced by the 4.3 version of h5bp?

@Gta-Cool
Copy link
Contributor Author

Ok, great!

No, last night, I forgot to update the twig with 4.3.* changes, I'll make modifications you have told me.

Do you agree with the use of jQuery 2.* ? Because the composer.json of "components/html5-boilerplate" have no superior restriction for jquery version, maybe I'll make a PR about it.

@lmammino
Copy link
Member

Cool, I can't wait to release this new version of the boilerplate! 😉

For what regards jQuery 2.* i don't agree at all... mostly for the following two reasons:

  • "jQuery 2.x has the same API as jQuery 1.x, but does not support Internet Explorer 6, 7, or 8" as stated in the jQuery documentation... I think older IE version still have a good market share, so we should preserve those who still want to develop web sites that works also for these old platforms.
  • Actual version of h5bp adopts jQuery 1.10.2 and I would love to strictly follow their specifications to make the bundle an exact porting. I have already submitted a pull request to components/html5-boilerplate to force the 1.10.2 version (Updated jQuery to 1.10.2 components/html5-boilerplate#3)

…1.10.2 dependency in composer.json temporarily
@Gta-Cool
Copy link
Contributor Author

:)

The twig file has been updated with your recommendantions and I add a dependency to jquery 1.10.2 in composer.json temporarily (the time your pull request is accepted).

@lmammino
Copy link
Member

Tested!

I can confirm it works (i just only had some issues because https://github.com/robloach/component-installer requires a fresh version of composer)

Proceding with merge ;)

lmammino added a commit that referenced this pull request Sep 23, 2013
- Static dependencies have been removed and replaced by components (requires a fresh version of composer)
- Updated to H5BP 4.3
@lmammino lmammino merged commit c771961 into Oryzone:master Sep 23, 2013
@Gta-Cool Gta-Cool deleted the components branch September 23, 2013 13:25
@Gta-Cool
Copy link
Contributor Author

Great :)

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.

None yet

2 participants