Skip to content
This repository has been archived by the owner on Jan 1, 2020. It is now read-only.

Improvements and fixes #339

Merged
merged 8 commits into from
Jun 15, 2016
Merged

Improvements and fixes #339

merged 8 commits into from
Jun 15, 2016

Conversation

michalbundyra
Copy link
Member

  • fix image alt to use version from Application\Module instead of fixed 2 version
  • changes short echo notation to normal echo (consistency with all other places in skeleton)
  • updated bootstrap to latest version 3.3.6
  • removed redundant/outdated JS assets

suggestion: maybe will be better to use CSS from cdn server instead including it into project?

@alextech
Copy link
Contributor

alextech commented Jun 10, 2016

CDN good for production build, not for develop. Frequent refreshes in local testing often get slowed down for me when DNS is not too happy being bombarded by me. Unpredictable external resource that may change for a public library also does not feel safe. One thing when its me maintaining it for my application, another when we make that assumption in publicly distributed library. We need skeleton to be as reliable as possible at any point of time and place.

That said, having all those JS assets part of the source base is ugly too, but its probably lesser of three evils, another one being introducing JS package manager calls from composer install which would be "proper" way to do it but beyond skope of skeleton's purpose.

On note of removing JS files. Is bootstrap complete without them? I am not 100% sure on this one but I would imagine the expectation users would have seeing "bootstrap" in sources is they can use all of its features, not just visual styles.

Then there is removing full sources and leaving minified only. Since this skeleton is a combo of UI HTML elements, not just API like apigility is, users are going to want to reliably debug their frontends, which means having full sources or at least source maps along with it. (issues JS/CSS package managers solve but i doubt we want to go anywhere near that here)

@michalbundyra
Copy link
Member Author

I don't like including complied bootstrap files into the project at all. I'm agree that not complete bootstrap (without JS) could be confusing. But what is the point including external (and not used or even outdated) libraries into the project? Maybe then ZendSkeletonApplication shouldn't have any other CSS/JS libraries included, only base style file?

Anyway, I think it would be nice to use some packages manger (npm/bower ...?) with gulp with new ZendSkeletonApplication. If we have default configuration of gulp which will compile SASS/JS files and create revision manifest file (to prevent browser caching), it could speed up building new application. In modern application probably everyone need to do that, so it will be nice if this functionality will be delivered out-of-the-box (or at least in some separate zend module). It will also resolve problem with outdated resources in ZendSkeletonApplication, then will be not needed to include external libraries into the repository at all.

I've prepared new Asset view helper - zendframework/zend-view#64. It could be used with revision manifest file generated by gulp/grunt or just array resource map from configuration.

I'm happy to create PR with npm+gulp configuration for the ZendSkeletonApplication if you think it's good idea.

@alextech
Copy link
Contributor

alextech commented Jun 12, 2016

Tl;Dr, great idea that would be good to see in Zend-View repository

But what is the point including external (and not used or even outdated) libraries into the project? Maybe then ZendSkeletonApplication shouldn't have any other CSS/JS libraries included, only base style file?

Its a compromise between having no UI framework at all making starting a visual MVC more awkward than it has to be, and having too much UI library maintenance taking away from focus of this being a backend library first - frontend optional. We cannot predict what will users want, bootstrap, dojo, some custom thing altogether from themeforest, or even none at all with pure JSON API (half the time I just kill the view folder of skeleton straight away). Those who care can swap or upgrade their JS libraries easy enough as is. That is not our specialty here so probably should not bother with that. Some basics are given to get a general idea of how MVC is out to be structured with inspirational pretty picture as opposed to ugly HTML alone, but it is not to be taken as a UI development kit with npm/gulp (also not forget being a server framework first, UI second, we are not driving decision of the user to must have npm vs bower vs Sencha CMD which I currently use). It works out of the box as a utility to make the pages look pretty on step1, but not our responsibility beyond that.

"Anyway, I think it would be nice to use some packages manger"

yes, not with skeleton though but addon plugin to zend view side of zend-mvc. It is a hard question merging the two together. I wondered many times how to have the two library managers work together - have composer and zend take care of php stuff, npm run around the view files and manage JS/CSS dependencies without splitting the project into two subprojects, one in source root, another in /public . I think your idea is great as an addon to skeleton - if such thing was available it would have solved me some problems in the past, but not within it because I use the skeleton for non-UI starting points too.

It will also resolve problem with outdated resources in ZendSkeletonApplication, then will be not needed to include external libraries into the repository at all.

Good point, if this was UI centric development kit. For what skeleton is, I don't see importance of keeping updated assets in skeleton. I am happy to use any version of bootstrap, outdated or not, I would not even bother checking, since I am here to write PHP code.

That said, the opposite is true in zend-view. There, I do care very much of how up to date my assets are. Send your proposal to that repository and it should be very helpful and welcome there I would hope. Zend-View could use some modernization to bring it inline with how templates are written these days.

@@ -4,7 +4,7 @@
<p>
Congratulations! You have successfully installed the
<a href="https://github.com/zendframework/ZendSkeletonApplication" target="_blank">ZF Skeleton Application</a>.
You are currently running Zend Framework version <?= \Application\Module::VERSION ?>.
You are currently running Zend Framework version <?php echo \Application\Module::VERSION ?>.
Copy link
Member

Choose a reason for hiding this comment

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

<?= is fine, as our target is 5.6+ (5.4+ has that always work, regardless of the short_tags configuration setting).

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I know, I just want to be consistent - in other views we are always using <?php echo

Copy link
Member

Choose a reason for hiding this comment

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

Well, then, I guess I better fix those. 😄 (I've been trying to convert those whenever I see them.)

@weierophinney
Copy link
Member

We have shipped Bootstrap directly in the skeleton for a variety of reasons:

  • First, as noted in the thread, it provides something reasonable for developers to see out-of-the-box.
  • Second, it provides a CSS framework out-of-the-box for developers to use while prototyping. Yes, we could use a simple custom CSS file, or a slimmer CSS framework, but we chose Bootstrap in large part because it is a framework targeting specifically back-end developers, who may or may not have much experience or expertise with front-end development. Bootstrap is also quite well-known by back-end developers.
  • Third, it provides a common target for our tutorials. In point of fact, I just finished up refactoring the bulk of our tutorials, and one common element in them is showing how to customize output in your view scripts; having a CSS framework in place already provides an excellent target for this, and I'm able to demonstrate customizing form generation, navigation menus, breadcrumbs, and even pagers. If I had to also provide and explain CSS along the way, this would be a nightmare.
  • Fourth, as @alextech notes, we ship the full thing, and do not use a CDN, to allow for minimal bandwidth and offline development. This is more crucial than you might think; bandwidth is not universal, and we've had complaints before even about having a PHPUnit requirement in our packages, as some developers do not like the fact that they end up installing it for every package they contribute to! (Fortunately, Composer's caching ability has made that less painful!) Additionally, as somebody who regularly presents at conferences and delivers workshops, not having to rely on an internet connection is a huge boon.
  • Fifth, we ship both the full version and the minified versions. The reason for this is to ensure developers can fall-back to the full version if they need to debug CSS and/or JS interactions, but use the minified version for basic development and performance. Having not just the bootstrap CSS but also the JS ensures that developers can use anything documented on the Bootstrap site.

The choice of Bootstrap can definitely be problematic at times, but I hope the above illustrates why we use it in the skeleton. We have friends who have also detailed that Bootstrap is great for prototyping... but should be ripped out once the front-end requirements have stabilized, to be replaced with something bespoke. As such, it's present to help users get started building something.

We haven't shipped with front-end tooling to date. Originally, this was because it didn't really exist. Currently it's because there's too many to choose from, and if you're going to need that, you likely need to work with your front-end developers to incorporate the tooling they prefer and/or which works best for the project.

Overall, this patch looks good; I'm going to pull it locally, and test it out with a test project (which builds everything in all of our tutorials) to ensure it still does what we expect; if not, I'll provide feedback.

@weierophinney
Copy link
Member

I'm a little worried that the bootstrap JS is gone; it means developers would not be able to make immediate usage of common features such as tabs, modals, and dropdowns (this latter is a target for navigation, for instance).

Since I know the version you're targeting, mind if I re-add the bootstrap JS (and, likely, jquery) during merge?

@michalbundyra
Copy link
Member Author

Ok, I'll update this PR in a second, with JS files and jQuery 3.0 :)

@weierophinney
Copy link
Member

@webimpress Awesome, thanks!

@alextech
Copy link
Contributor

Good change regardless what I said, because starting fresh from outdated frontend libs is no fun and looks bad. +1 for adding JS back.

Logistics question. Chasing bootstrap/JQuery changes can be done indefinitely. Can @webimpress do another PR like this again just before this package gets fully released? Doing them over and over throughout dev cycle may not be as productive, but one last greatest and latest commit during release date would be very helpful, I think.

@weierophinney
Copy link
Member

@alextech This will be released quite soon (as in within the next couple of weeks). We can update later as new versions come out if we need to; most of the time, there's no real need to chase the latest version in the skeleton unless there are critical fixes that could impact developers. (As proof: I didn't have to change a single bit of our markup going from whatever we have currently to what @webimpress has provided!)

@alextech
Copy link
Contributor

Ok, that is indeed close enough for any new changes. I was thinking if another quarter or something long.

@michalbundyra
Copy link
Member Author

Oh, unfortunately Bootstrap 3.3.6 is not working with jQuery 3.0.0 :(

bootstrap.min.js:6 Uncaught Error: Bootstrap's JavaScript requires jQuery version 1.9.1 or higher, but lower than version 3

@michalbundyra
Copy link
Member Author

@weierophinney it's done now.

@weierophinney weierophinney added this to the 3.0.0 milestone Jun 15, 2016
@weierophinney weierophinney self-assigned this Jun 15, 2016
@weierophinney weierophinney merged commit dbf6ff2 into zendframework:develop Jun 15, 2016
weierophinney added a commit that referenced this pull request Jun 15, 2016
weierophinney added a commit that referenced this pull request Jun 15, 2016
weierophinney added a commit that referenced this pull request Jun 15, 2016
@weierophinney
Copy link
Member

Thanks, @webimpress !

@michalbundyra michalbundyra deleted the improvements-and-fixes branch June 16, 2016 07:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants