Update Satis UI to Bootstrap 4 #512

antoniocambados · 2018-10-21T17:51:12Z

This PR freshens up the Satis UI by upgrading it from Bootstrap 3 to Bootstrap 4. It also updates the JavaScript code, which has been converted to modern ES6 classes.

Some things worth noting:

Both CSS and JS have been compiled using Webpack (with Webpack Encore for an easier setup).
Overall assets size has dropped from ~280Kb to ~180Kb.
Contains a longer explanation on how to set up the repository, which I largely took from here.

Here's how it looks:

And here is the detail of the setup info:

- Uses Bootstrap 4.1 - Migrates javascript to ES6 modules and classes - Removes dependency from the massive moment.js library

Applies a more streamlined and simple layout, discarding the previous navbar-like style.

codecov-io · 2018-10-21T18:36:11Z

Codecov Report

Merging #512 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #512   +/-   ##
=========================================
  Coverage     51.78%   51.78%           
  Complexity      373      373           
=========================================
  Files            11       11           
  Lines          1064     1064           
=========================================
  Hits            551      551           
  Misses          513      513

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73d0aa4...dbfc088. Read the comment docs.

alcohol · 2018-10-22T06:37:13Z

I think the /build/ output should be added to .gitignore, should it not? We can compile it in Travis before creating a container.

staabm · 2018-10-22T07:38:38Z

I really like updating the UI.

IMO you should make sure that the page header does not occupy that much space.
we recently updated the UI to use less space, see e.g.
#506
#505

alcohol · 2018-10-22T12:06:04Z

Removed some duplicate comments as a result of the major outage Github had with their storage backend.

stof · 2018-10-22T12:18:23Z

@alcohol this would hurt people installing Satis through composer, as they would not have the compiled assets

stof

You should update the contributing doc, to explain how to contribute to the frontend (as there is now a build step).

And it might make sense to setup Travis to ensure that assets were compiled properly.

package.json

views/assets/js/App/DateDistance.js

stof · 2018-10-22T12:26:29Z

webpack.config.js

+    // directory where compiled assets will be stored
+    .setOutputPath('views/build/')
+    // public path used by the web server to access the output path
+    .setPublicPath('/assets')


this is wrong. the public path is build, not assets (as you don't use the manifest, you are less impacted by the wrong config of course)

The compiled files should be relative to the the "output" option from the cli command.

@codecov-io the current WebBuilder embeds every asset in the html code (css, js, images), so it's not possible for those to be relative to the "output-dir" option of the build command.

One thing I had in mind was making the build command to copy the compiled assets (or maybe symlink them if it's possible) to the "output-dir" and referencing them rather than embedding them. If you all want, I can give it a go and add this to the PR as well.

alcohol · 2018-10-22T12:28:42Z

But should that not be part of their setup process (README should be updated)?

I mean, just a git clone is not sufficient for them either. They would still need to run composer install.

stof · 2018-10-22T12:29:43Z

Btw, if you want to reduce the file size of assets, an option might be to drop jQuery too.

alcohol · 2018-10-22T12:38:10Z

It would be nice to drop the jQuery dependency. Considering what little we use it for we could probably write that in vanilla JS just as well.

alcohol · 2018-10-22T12:48:03Z

If you're feeling generous/creative/productive/brave/optimistic, you could maybe also take a stab at #513

stof · 2018-10-22T12:58:14Z

@alcohol in which image ? For people installing Satis through composer, they download the github source, not an image.

antoniocambados · 2018-10-23T00:31:54Z

I think some messages have gone missing or misplaced. What a messy day on GitHub!

Regarding the compiled assets I think those should be included. If not, we would be forcing to compile them at least once. That could be done with a composer post-install script, but it would only succeed if the user has the required setup (node, npm, yarn...). And we're talking about a mandatory step here even if whoever installed this package does not intend to modify the UI whatsoever. Moreover, one could argue that the current version relies on and is including the compiled assets anyways 😅

antoniocambados · 2018-10-23T00:49:01Z

It would be nice to drop the jQuery dependency. Considering what little we use it for we could probably write that in vanilla JS just as well.

Well that could certainly be done, but maybe replacing jQuery with a lightweight alternative such as cash is not a bad idea either. Overall assets size would be ~100Kbs

alcohol · 2018-10-23T05:56:55Z

I am honestly not familiar with the current trend of javascript frameworks (both small and large) as I rarely do frontend development, so I'll defer to your judgement.

It also sounds like committing the compiled assets makes more sense, so keep it like that. Would you like to add any follow up work to this same PR? Or shall I go ahead and merge and then you can submit any other work in a new PR (or multiple even)?

staabm · 2018-10-23T07:12:52Z

package.json

+    "@symfony/webpack-encore": "^0.20.0",
+    "bootstrap": "^4.1.3",
+    "date-fns": "^1.29.0",
+    "jquery": "^3.3.1",


would it make sense to use jquery-slim in this spot (as we used the slim variant also before this PR)?

or do some parts of bootstrap require the "full build"?

changing jquery with a different lib can be done in a later PR when usefull.

The jquery-slim version is not offered as an npm package. The jquery package contains a compiled version of jquery-slim though, but funnily, if you require that instead of the regular jquery, the bundled js is significantly larger.

Bootstrap 4 requires jquery in order to run it's javascript features, in addition to popper if you want to use popups, tooltips and the like.

As of now, the PackageFilter class (which I ported directly from the previous code) makes heavy use of jquery, and I've also introduced the use of Bootstrap's collapsible plugin.

I've been working a bit on this but I think maybe it's better to be done in another PR once this is merged.

staabm · 2018-10-23T07:13:45Z

It also sounds like committing the compiled assets makes more sense, so keep it like that

I agree with it. in the past I just cloned satis from github and was done with it. no build required.

staabm · 2018-10-23T07:14:21Z

I really like updating the UI.

IMO you should make sure that the page header does not occupy that much space.
we recently updated the UI to use less space, see e.g.
#506
#505

any feedback regarding my above comment?

staabm · 2018-10-23T07:16:04Z

Well that could certainly be done, but maybe replacing jQuery with a lightweight alternative such as cash is not a bad idea either. Overall assets size would be ~100Kbs

in the past, the satis website didn't have any external depedencies. everything was embedded in the html document. not sure why, but maybe this was a "feature" which we need @alcohol ?

alcohol · 2018-10-23T07:33:29Z

in the past, the satis website didn't have any external depedencies. everything was embedded in the html document. not sure why, but maybe this was a "feature" which we need @alcohol ?

I think it was mostly a matter of simplicity. It is not a requirement as far as I know.

antoniocambados · 2018-10-23T08:18:26Z

I think enabling external dependencies would make it easier to improve the web UI. I'll be doing another PR with this soon.

antoniocambados · 2018-10-23T08:32:20Z

I really like updating the UI.
IMO you should make sure that the page header does not occupy that much space.
we recently updated the UI to use less space, see e.g.
#506
#505

any feedback regarding my above comment?

I can work a bit on that 😃

antoniocambados · 2018-10-23T08:33:31Z

I am honestly not familiar with the current trend of javascript frameworks (both small and large) as I rarely do frontend development, so I'll defer to your judgement.

It also sounds like committing the compiled assets makes more sense, so keep it like that. Would you like to add any follow up work to this same PR? Or shall I go ahead and merge and then you can submit any other work in a new PR (or multiple even)?

Before merging, as @stof pointed out I'll be fixing the manifest and updating the docs for frontend contribution instructions, plus I'll try to make the header more compact too as I posted above.

SvenRtbg · 2018-10-23T08:52:36Z

I'd prefer not to link to external resources if possible. That way the page always fully works, even in internal networks with limited remote connections.

staabm · 2018-10-23T09:09:18Z

I'd prefer not to link to external resources if possible. That way the page always fully works, even in internal networks with limited remote connections.

to be more precise: I meant external js/css files on the same server host, not remote css/js files.

the current version of satis embedds everything into the html document.
I dont want it to request resources which are only available from a host which is connected to the internet.

alcohol · 2018-10-23T09:09:34Z

None of these resources are external. They are all local to the project. The only change is that the javascript is no longer completely embedded within the html, but instead served separately.

- More compact initial design. - More and clearer information in the setting up info section.

antoniocambados · 2018-10-23T22:13:37Z

Fixed the configuration for the (unused) manifest.json and brought to you a new header I'm really happy with: sleek, more compact yet with more info and useful links. Have a look:

alcohol · 2018-10-24T07:05:28Z

Nicely done. I'll merge this to keep the changeset relatively small. Any follow up work, please submit a new PR. I will happily review and merge those too :-)

staabm · 2018-12-21T09:03:57Z

just pulled the latest master into our instance but didn't got the new design.

is a build step or similar required?

staabm · 2018-12-21T09:13:41Z

ohh I had to hard-refresh the page in the browser.

I don't know twig very well, but maybe we could add a cache-buster into the url of the stylesheets changed with this PR ?

antoniocambados · 2018-12-29T12:46:07Z

@staabm Satis does not reference stylesheets, JS or icons, it embeds them in the HTML response. I was going to separate assets in this PR, but decided to go for that in a follow-up PR, focused on making satis easier to customize, because not having to build satis for every CSS change is a huge advantage for that matter.

So, coming back to your issue, I guess either your browser or your server had he HTML document cached.

staabm · 2018-12-29T12:52:25Z

@staabm Satis does not reference stylesheets, JS or icons, it embeds them in the HTML response. I was going to separate assets in this PR, but decided to go for that in a follow-up PR, focused on making satis easier to customize, because not having to build satis for every CSS change is a huge advantage for that matter.

Totally agree

So, coming back to your issue, I guess either your browser or your server had he HTML document cached.

Maybe a workaround with the Clear-Site-Data header could work.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Clear-Site-Data
But maybe its also more like an edge case which shouldnt be considered.

Thx for the followup

antoniocambados added 7 commits October 20, 2018 19:09

Add Webpack configuration.

a7880ba

Redesign.

13aeb10

- Uses Bootstrap 4.1 - Migrates javascript to ES6 modules and classes - Removes dependency from the massive moment.js library

Optimizes CSS file size by disabling unused Bootstrap features.

49d6fd2

Changes the layout for the header section.

6fa7b8c

Applies a more streamlined and simple layout, discarding the previous navbar-like style.

Simpler package properties markup.

2873519

Minor js tweaks.

1bef82b

Bugfix: there are elements with wrong markup.

90318c3

antoniocambados changed the title ~~Bootstrap 4~~ Update Satis UI to Bootstrap 4 Oct 21, 2018

Bugfix: tests were failing.

7ce97d6

antoniocambados force-pushed the bootstrap-4 branch from 4ce172b to 7ce97d6 Compare October 21, 2018 18:33

composer deleted a comment from staabm Oct 22, 2018

stof reviewed Oct 22, 2018

View reviewed changes

antoniocambados added 2 commits October 22, 2018 21:31

Changes license of package.json to MIT.

70965d6

Remove jQuery dependency in DateDistance.js

9dedb23

antoniocambados closed this Oct 23, 2018

antoniocambados reopened this Oct 23, 2018

staabm reviewed Oct 23, 2018

View reviewed changes

antoniocambados added 2 commits October 24, 2018 00:03

Header redesign.

96edcba

- More compact initial design. - More and clearer information in the setting up info section.

Bugfix: the public path is 'build', not 'assets'.

dbfc088

alcohol merged commit 65d1f34 into composer:master Oct 24, 2018

Update Satis UI to Bootstrap 4 #512

Update Satis UI to Bootstrap 4 #512

Conversation

antoniocambados commented Oct 21, 2018

codecov-io commented Oct 21, 2018 • edited Loading

Codecov Report

alcohol commented Oct 22, 2018

staabm commented Oct 22, 2018

alcohol commented Oct 22, 2018

stof commented Oct 22, 2018

stof left a comment

Choose a reason for hiding this comment

stof Oct 22, 2018

Choose a reason for hiding this comment

flaviu-chelaru Oct 22, 2018

Choose a reason for hiding this comment

antoniocambados Oct 23, 2018

Choose a reason for hiding this comment

alcohol commented Oct 22, 2018

stof commented Oct 22, 2018

alcohol commented Oct 22, 2018

alcohol commented Oct 22, 2018

stof commented Oct 22, 2018

antoniocambados commented Oct 23, 2018

antoniocambados commented Oct 23, 2018

alcohol commented Oct 23, 2018

staabm Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

antoniocambados Oct 23, 2018

Choose a reason for hiding this comment

staabm commented Oct 23, 2018 • edited Loading

staabm commented Oct 23, 2018

staabm commented Oct 23, 2018

alcohol commented Oct 23, 2018

antoniocambados commented Oct 23, 2018

antoniocambados commented Oct 23, 2018

antoniocambados commented Oct 23, 2018

SvenRtbg commented Oct 23, 2018 via email

staabm commented Oct 23, 2018 • edited Loading

alcohol commented Oct 23, 2018

antoniocambados commented Oct 23, 2018

alcohol commented Oct 24, 2018

staabm commented Dec 21, 2018

staabm commented Dec 21, 2018 • edited Loading

antoniocambados commented Dec 29, 2018

staabm commented Dec 29, 2018

codecov-io commented Oct 21, 2018 •

edited

Loading

staabm Oct 23, 2018 •

edited

Loading

staabm commented Oct 23, 2018 •

edited

Loading

staabm commented Oct 23, 2018 •

edited

Loading

staabm commented Dec 21, 2018 •

edited

Loading