Bug 1408100 - Upgrade to Bootstrap 4 #2883

camd · 2017-10-27T19:05:20Z

Transition from Bootstrap 3 to 4
Bug 1408100

Note: This is not pixel-perfect with Bootstrap 3. To do that would have meant overriding a lot of Bootstrap 4. I hope this is agree-able with everyone. As it is, I still had to do quite a bit of pixel-tweaking to get it to look nice again

I tried to split this into pieces that would take some of the noise out of it. The final 3 commits don't really stand on their own, so we could arguably squash those together. But this split should help with the review a bit.

I must apologize, this has been kind of messy to get us back close to where we were. We already had lots of pixel tweaks in our old CSS, and this is just some different pixel tweaks. :)

imbstack · 2017-10-27T19:14:28Z

Looks great! Notes from testing on stage:

Action menu items like "Add new jobs" are only clickable on the words themselves, not across the entire <li> element. The dropdown does close however, so it ends up being a bit confusing.
The top navbar seems to have some extra space between the bottom of the buttons and the bottom of the bar. Their appears to be extra padding or something when hovering for the highlight and clicking on them brings up a weird blue shadow that doesn't quite fit either.
The 'Other job actions' menu seems to be inside an extra ul/div or something? Highlights don't reach the whole way across when you hover and it kinda looks strange in general?

Other than those nits, everything seems to work how I would expect it to w.r.t. actions!

wlach · 2017-10-27T19:33:44Z

Some comments re perfherder:

"Add more test data" button in graphs view doesn't have a hand icon. Maybe: https://stackoverflow.com/questions/41913646/bootstrap-button-no-hand-cursor
New test chooser has some issues compared to old one. Notably:
labels (e.g. "Framework") look wrong (not bold anymore)
"Add test data" and close button positioning swapped in a non-standard way (close button should be on right, not left -- vice versa for title)
Form elements also seem larger than I would expect/need (compare old vs. new)
Tooltips in comparison view (e.g. https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=1f1b4cb86b5892bccae047959b480ae6b863a7fe&newProject=mozilla-inbound&newRevision=1c4b4486947b3af2e53ac489eb8a2caadbe66729&originalSignature=054905c426e16aca2e90a056152d32e9f9616511&newSignature=054905c426e16aca2e90a056152d32e9f9616511&framework=1) lost their pointers and got overly large font sizes. Before/after:

* Spacing between tooltips in graph details looks off (it's overly compact). Before/after:

* (most critical) Can't scroll down at all in alerts view: https://treeherder-prototype.herokuapp.com/perf.html#/alerts?hideDwnToInv=1 (I can't really give feedback on alerts until this is fixed)

I know some of this might sound picky but I spent quite a bit of time both writing the original CSS and reviewing other people's PRs to make sure information was formatted in the clearest possible way and it would kind of suck to lose that. I'm not saying that things have to look exactly the same as before, but it would be good to at least preserve the spirit of the original.

edmorley · 2017-10-27T22:10:52Z

package.json

    "angular-ui-router": "0.4.3",
-    "bootstrap": "3.3.7",
+    "angular1-ui-bootstrap4": "^2.4.22",
+    "bootstrap": "4.0.0-beta",


Beta 2 is out, let's use that :-)

$ npm view bootstrap dist-tags { latest: '3.3.7', next: '4.0.0-beta.2' }

https://blog.getbootstrap.com/2017/10/19/bootstrap-4-beta-2/

edmorley · 2017-10-27T22:12:42Z

package.json

    "angular-ui-router": "0.4.3",
-    "bootstrap": "3.3.7",
+    "angular1-ui-bootstrap4": "^2.4.22",


For consistency with the other entries (and to make it easier to see what needs updating, given lack of tools compatibility with reading yarn.lock for outdatedness), I think we should remove the ^.

edmorley · 2017-10-27T22:13:19Z

package.json

@@ -39,6 +39,7 @@
    "neutrino-preset-react": "4.2.3",
    "ngreact": "0.5.0",
    "numeral": "2.0.6",
+    "popper.js": "^1.12.5",


The caret can be removed here too (and adjust to 1.12.6)

edmorley · 2017-10-27T22:26:42Z

ui/entry-failureviewer.js

@@ -19,8 +19,9 @@ require('angular-resource');
 require('angular-cookies');
 require('angular-sanitize');
 require('angular-local-storage');
+require('popper.js/dist/umd/popper');
 require('bootstrap/dist/js/bootstrap');


Let's switch the bootstrap entry to use import and the format recommended here:
https://getbootstrap.com/docs/4.0/getting-started/webpack/#importing-javascript

Also, that page doesn't say we have to also require('popper.js/dist/umd/popper'), were things not working without that?

Yeah, you're right. I took it out and it still worked. I was hitting so very many problems earlier on that I think I'd tried that and hadn't taken it out yet... :) Thanks for catching that.

wlach · 2017-10-30T15:52:37Z

@camd the pagination now looks ok in the alerts view and seems to work ok, though we lost the padding around it. I'm imagining you just hadn't gotten around to fixing that yet... (perhaps it's just a matter of making sure an existing css class is applied?)

wlach · 2017-10-30T16:33:57Z

Actually, now that I look at it, there's many more issues with padding and spacing on the perfherder alerts view. A visual comparison between old/new should make them obvious.

camd · 2017-11-04T00:57:22Z

I can see that a Selenium test failed. I'll address that Monday. :/

camd · 2017-11-04T19:11:31Z

ui/css/treeherder-resultsets.css

@@ -187,3 +205,19 @@ fieldset[disabled] .btn-resultset:hover {
 .job-list table tr:nth-child(even) {
  background: #f8f8f8;
 }
+
+.get-next {


Bootstrap 4 does away with the concept of a well so this adds the styling to get it to look like that again.

Bootstrap 4 no longer contains Glyphicons by default, so if we switch to Font Awesome, then that's one less set of icons we need to download.

These styling classes are still backward-compatible with BS-3 But remove some noise from the later commits.

wlach · 2017-11-06T17:40:08Z

"runs" tooltip looks strange in new version (has a down arrow pointing at wrong spot). I'd say either center the arrow if possible, or just fall back to the tooltips without arrows. Example URL: https://treeherder-prototype.herokuapp.com/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=209df98be4672182897eb92c2b7b807b2f7901b7&newProject=mozilla-inbound&newRevision=e9349ad2f1f8fec862b1d2271d0d8f25ad0814d4&originalSignature=f7a7eb10a31d949a8a02f1cee45a5ef452889bf7&newSignature=f7a7eb10a31d949a8a02f1cee45a5ef452889bf7&framework=1

Revisions text misaligned in alerts view:

Link: https://treeherder-prototype.herokuapp.com/perf.html#/alerts?hideDwnToInv=1

wlach · 2017-11-06T17:55:16Z

Not seeing hand cursor on close button on modals. E.g.:

wlach · 2017-11-06T17:58:48Z

Tooltips also look misaligned/weird in alerts view. E.g.:

wlach · 2017-11-06T22:14:25Z

ui/css/perf.css

@@ -576,7 +577,7 @@ li.pagination-next .material-icons::before {
  width: 0;
  height: 0;
  position: absolute;
-  margin-left: 80px;
+  margin-left: 47%;


Fixed pixel values would be better than percentage ones here, it's easier to reason about what they're doing.

I needed to opt for a percentage because the popup could be a different height/width, depending on content. Sometimes it has text of "Confidence value as calculated by Perfherder alerts. Note that...." and sometimes it's just "4 base / 4 new". With the pixel value, the shorter popup has the arrow way off to the side. This at least keeps it very close to the center.

Oh, the arrow positioning is based on this value? That's frustrating. :(

Yeah, the angular1-ui-bootstrap4 package removed the HTML element that was used for the arrow. Maybe they found it hard and just removed it to make things easier on themselves. Not sure. So I had to get all fancy-pants with a CSS after element to put it back.

What's the downside of doing it this way?

How about we just kill the arrow for now, since a bunch of other elements don't have them anyway. We can always reintroduce them when/at the point that we port the compare view widget to react.

camd · 2017-11-06T23:31:49Z

@wlach I addressed all the items you mentioned above. Though not sure it'll still be deployed to prototype by the time you look. I'll push it out there first thing tomorrow.

edmorley · 2017-11-07T18:23:56Z

I notice both before and after this PR, Treeherder is using:
https://github.com/mozilla/treeherder/blob/master/ui/vendor/css/bootstrap-non-responsive.css

On https://getbootstrap.com/docs/4.0/migration/#global-changes there is:

Non-responsive usage of Bootstrap is no longer supported.

As such, it seems like we shouldn't be using that file any more?

Note also that the "taken from" comment URL in our copy of bootstrap-non-responsive.css now 404s, the new link appears to be:
https://getbootstrap.com/docs/3.3/examples/non-responsive/non-responsive.css

...but the CSS seems to have been updated since our copy. See also these pages from the Bootstrap 3 docs for reference:
https://getbootstrap.com/docs/3.3/getting-started/#disable-responsive
https://getbootstrap.com/docs/3.3/examples/non-responsive/

The only mention Bootstrap 4 now has about responsiveness appears to be here:
https://getbootstrap.com/docs/4.0/getting-started/introduction/#responsive-meta-tag

...our viewport meta doesn't match what they suggest there, should it?

edmorley · 2017-11-07T18:35:15Z

I ran bootlint using their bookmarklet against a loaded mozilla-inbound page via yarn start and it found a handful of errors. I'm not sure if any were pre-existing or not, but might be worth checking?
https://github.com/twbs/bootlint#in-the-browser

edmorley · 2017-11-07T18:38:25Z

Hmm though looks like the current stable bootlint doesn't yet have official bootstrap 4 support, so perhaps not worth worrying about for now:
twbs/bootlint#410

camd · 2017-11-07T20:07:34Z

Thanks for catching the stuff about the non-responsive css. I removed it. I don't think it was doing much as things seemed the same with and without. I do think we should go with the viewport setting they suggested. I updated that as well.

I also removed the arrows from the popups as @wlach suggested.

edmorley

This is great! It must have been an utter pain to do as well!

Worth noting is that Bootstrap 4 dropping glyphicons means a 200KB and 5 file reduction in the built payload (only 1 of the five files is loaded, since the others are just variations for older browsers, but still one less HTTP request).

The only things I could find were:

the "i" button on the pinned repos (that shows links to tree status etc) -> when hovering over the links they no longer highlight (in comparison the links on the help menu still do highlight) - not a massive issue, up to you
the repos menu: some rows are no longer two columns wide, plus the links are a different colour and there is now a hover highlight per group (the latter two not a big deal, I'll leave that up to you)
the help and login menus downward arrows are of a larger style (eg almost bolded) compared to the downward arrow for the infra/repos/filters menus, though this isn't critical
log viewer: the menu to switch back to Treeherder is broken slightly
the "failure classification" panel's "edit" links are no longer styled like links
the job panel behaves differently when the window is at a narrow width (scroll bars, white strip down the right hand side of the window), plus the "annotations" and "similar jobs" panel tabs overflow into the panel content (though the behaviour wasn't exactly great before either, so perhaps this can be ignored) - see screenshots:

edmorley · 2017-11-07T22:12:48Z

neutrino-custom/base.js

@@ -166,6 +166,7 @@ module.exports = neutrino => {
            jQuery: require.resolve('jquery'),
            'window.$': require.resolve('jquery'),
            'window.jQuery': require.resolve('jquery'),
+            Popper: ['popper.js', 'default'],


Could you add a comment saying require by bootstrap and linking to https://getbootstrap.com/docs/4.0/getting-started/webpack/ ? :-)

edmorley · 2017-11-07T22:15:22Z

ui/entry-index.js

@@ -7,7 +7,6 @@ require('./js/config');
 // Styles
 require('bootstrap/dist/css/bootstrap.css');
 require('font-awesome/css/font-awesome.css');
-require('./vendor/css/bootstrap-non-responsive.css');


The file can now be removed from the repo :-)

ahh, yep. removed

edmorley · 2017-11-07T22:19:01Z

ui/index.html

@@ -3,7 +3,7 @@
    <head>
        <meta charset="utf-8">
        <title ng-bind="getWindowTitle()">Treeherder</title>
-        <meta name="viewport" content="width=device-width, initial-scale=1.0">
+        <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">


Should the other pages have a viewport meta too?

They weren't there before, so I hadn't added them. But we might as well.

edmorley · 2017-11-08T17:36:18Z

the repos menu: some rows are no longer two columns wide, plus the links are a different colour and there is now a hover highlight per group (the latter two not a big deal, I'll leave that up to you)

Screenshot to explain this one a bit better :-)

camd · 2017-11-08T23:39:02Z

@edmorley : I pushed up the latest changes on prototype. I tested this with the only windows VM I have: Windows 7... :) It looked good there and I didn't see the gaps you were describing. I presume you're on windows 10, though.

edmorley · 2017-11-08T23:56:32Z

It only occurs with narrow window sizes I think

camd · 2017-11-09T00:30:13Z

Yeah, I was trying with really narrow window sizes, kind of like the screenshots. But didn't look wrong in the way your screenshot did. :) I ordered a copy of windows 10 so I'll have it for testing in a VM. Not sure how long that will take...

edmorley · 2017-11-09T00:33:20Z

I don't know if this is of any use?
https://developer.microsoft.com/en-us/windows/downloads/virtual-machines

Also, if it helps, I'm using Nightly.

edmorley · 2017-11-09T00:35:25Z

Or this might be a smaller download?
https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/

This adds the upgrade to Bootstrap 4, and some basic changes and some CSS tweaks we needed to keep out UI consistent. The simpler changes are things like: * Classes that were renamed * Adding classes that are now needed (dropdown-item, etc) * Change an item from a button to a span * Changing order of items (modal header close button, etc) * CSS class syntax changes The other changes are lots of CSS padding, margin, font and other spacing tweaks.

camd mentioned this pull request Oct 27, 2017

Bug 1408100 - Upgrade to Bootstrap 4 #2823

Closed

camd temporarily deployed to treeherder-prototype October 27, 2017 19:54 Inactive

edmorley reviewed Oct 27, 2017

View reviewed changes

camd temporarily deployed to treeherder-prototype October 27, 2017 22:48 Inactive

camd temporarily deployed to treeherder-prototype October 28, 2017 00:51 Inactive

camd temporarily deployed to treeherder-prototype October 30, 2017 16:36 Inactive

camd force-pushed the bootstrap-4-modals branch 2 times, most recently from 8a9a38d to 0561f72 Compare October 31, 2017 18:03

camd had a problem deploying to treeherder-prototype October 31, 2017 18:03 Failure

camd temporarily deployed to treeherder-prototype October 31, 2017 19:08 Inactive

camd force-pushed the bootstrap-4-modals branch 3 times, most recently from af74dfc to 9d373b7 Compare November 4, 2017 00:10

camd temporarily deployed to treeherder-prototype November 4, 2017 00:11 Inactive

camd force-pushed the bootstrap-4-modals branch from 9d373b7 to e346b9a Compare November 4, 2017 00:42

camd commented Nov 4, 2017

View reviewed changes

edmorley mentioned this pull request Nov 4, 2017

Bug 1408100 - Remove angular ui bootstrap #2881

Closed

Cameron Dawson added 3 commits November 6, 2017 09:28

Bug 1408100 - Some trivial syntax and typo fixes

6c7a36f

Bug 1408100 - Switch from glyphicons to fontawesome

f75a428

Bootstrap 4 no longer contains Glyphicons by default, so if we switch to Font Awesome, then that's one less set of icons we need to download.

Bug 1408100 - Add styling classes that won't exist in Bootstrap 4

7a98d28

These styling classes are still backward-compatible with BS-3 But remove some noise from the later commits.

camd force-pushed the bootstrap-4-modals branch from e346b9a to 962dfec Compare November 6, 2017 17:29

camd temporarily deployed to treeherder-prototype November 6, 2017 17:29 Inactive

wlach reviewed Nov 6, 2017

View reviewed changes

camd temporarily deployed to treeherder-prototype November 6, 2017 23:19 Inactive

camd temporarily deployed to treeherder-prototype November 6, 2017 23:46 Inactive

camd force-pushed the bootstrap-4-modals branch from 6f5d626 to aabc6ec Compare November 7, 2017 17:10

camd temporarily deployed to treeherder-prototype November 7, 2017 17:16 Inactive

camd temporarily deployed to treeherder-prototype November 7, 2017 20:08 Inactive

edmorley approved these changes Nov 8, 2017

View reviewed changes

camd temporarily deployed to treeherder-prototype November 8, 2017 17:45 Inactive

camd temporarily deployed to treeherder-prototype November 8, 2017 18:30 Inactive

camd force-pushed the bootstrap-4-modals branch from 9d5aa59 to 21db3ff Compare November 8, 2017 18:49

camd temporarily deployed to treeherder-prototype November 8, 2017 18:49 Inactive

camd temporarily deployed to treeherder-prototype November 8, 2017 23:09 Inactive

camd force-pushed the bootstrap-4-modals branch from 49aeb75 to 830e642 Compare November 8, 2017 23:31

camd temporarily deployed to treeherder-prototype November 8, 2017 23:31 Inactive

camd temporarily deployed to treeherder-prototype November 9, 2017 01:04 Inactive

camd force-pushed the bootstrap-4-modals branch from c3a0260 to 281d00a Compare November 9, 2017 19:06

camd merged commit 51dc3cc into master Nov 9, 2017

camd deleted the bootstrap-4-modals branch November 9, 2017 19:12

Bug 1408100 - Upgrade to Bootstrap 4 #2883

Bug 1408100 - Upgrade to Bootstrap 4 #2883

Conversation

camd commented Oct 27, 2017 • edited Loading

imbstack commented Oct 27, 2017

wlach commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlach commented Oct 30, 2017

wlach commented Oct 30, 2017

camd commented Nov 4, 2017

Choose a reason for hiding this comment

wlach commented Nov 6, 2017

wlach commented Nov 6, 2017

wlach commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camd commented Nov 6, 2017

edmorley commented Nov 7, 2017 • edited Loading

edmorley commented Nov 7, 2017

edmorley commented Nov 7, 2017

camd commented Nov 7, 2017

edmorley left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmorley commented Nov 8, 2017

camd commented Nov 8, 2017

edmorley commented Nov 8, 2017

camd commented Nov 9, 2017

edmorley commented Nov 9, 2017

edmorley commented Nov 9, 2017

camd commented Oct 27, 2017 •

edited

Loading

edmorley commented Nov 7, 2017 •

edited

Loading

edmorley left a comment •

edited

Loading