Upgrade to Bootstrap 4.0.0.alpha6 #1816

jhawthorn · 2017-04-03T20:09:51Z

Previously we were on Bootstrap 4.0.0.alpha2. Changes can be seen in their Release notes

Notable to us:

col-xs-3 et al. are gone. Replaced with col-3.
Flexbox is on by default 👍 (we had already turned on the optional flexbox support)
Classes for breadcrumbs changed (easy fix)
Some variables and mixins changed

I can't be 100% sure this didn't mess up any styling, but in a quick browse through the admin I didn't spot anything off.

Previously normalize.css set table's border-spacing to 0, bootstrap now uses reboot.css, which doesn't reset this property.

Bootstrap has removed col-xs-N in the latest alphas.

This works around a bootstrap 4.0.0.alpha6 issue resulting in Error: Tooltip is transitioning We should be able to re-enable this for the next release if desired.

tvdeyen

Found some stuff, that's partially related to these changes, but should be tackled in this PR IMO.

Feel free to grab from https://github.com/tvdeyen/solidus/commits/bootstrap-4-a6-review where you see fit.

tvdeyen · 2017-04-04T08:39:51Z

backend/app/assets/stylesheets/spree/backend/_bootstrap_custom.scss


 // Forms

 $input-padding-x:                .75rem !default;
-$input-padding-y:                .375rem !default;
+$input-padding-y:                .5rem !default;
+$input-line-height:              1.25 !default;


We need to add the newly introduced $input-line-height variable into our custom form fields, otherwise the inputs are too high in contrast to the selects

Before

After

input[type="text"], ... { ... font-size: $font-size-base; line-height: $input-line-height; ... }

tvdeyen · 2017-04-04T08:52:30Z

backend/app/views/spree/admin/orders/_add_product.html.erb

@@ -2,7 +2,7 @@
  <fieldset class="no-border-bottom">
    <legend align="center"><%= Spree.t(:add_product) %></legend>

-    <div class="col-xs-9">
+    <div class="col-9">


I think this wrapper should be removed altogether. I know this is not part of this PR, but with the removal of the xs column classes this gets more obvious then before.

Before

After

tvdeyen · 2017-04-04T08:53:31Z

backend/app/views/spree/admin/orders/_add_line_item.html.erb

@@ -2,7 +2,7 @@
  <fieldset class="no-border-bottom">
    <legend align="center"><%= Spree.t(:add_product) %></legend>

-    <div class="col-xs-9">
+    <div class="col-9">
      <div data-hook="add_product_name" class="field">
        <%= label_tag :add_line_item_variant_id, Spree.t(:name_or_sku) %>
        <%= text_field_tag :add_line_item_variant_id, "", class: "variant_autocomplete fullwidth" %>


Out of scope, but I couldn't find any reference to this partial. 🤔

tvdeyen · 2017-04-04T09:10:15Z

backend/vendor/assets/stylesheets/solidus_admin/bootstrap/_media.scss

-.media-list {
-  padding-left: 0;
-  list-style: none;
+.media-body {


Flexbox 💪

Changes to this file visually breaks the variant select in both Chrome and Firefox.

Adding

.media-left { padding: .5rem .5rem 0 0; }

fixed this for me.

Before

After

Right. I forgot this.

Bootstrap removed the .media-left class https://v4-alpha.getbootstrap.com/layout/media-object/ instead asking you to just specify .mr-3.

👍 Your padding solution is a good fix.

tvdeyen · 2017-04-04T09:22:04Z

backend/app/views/spree/admin/orders/_shipment.html.erb

@@ -50,7 +50,7 @@
        <tr class="edit-method hidden total">
          <td colspan="5">
            <div class="row">
-              <div class="col-xs-4">
+              <div class="col-4">


I'm not quite sure why this start to break visually now, but this makes the select too small. We should change this to col-12. We could also remove the row and col completely as I don't see any advantage to have this in this form, but this could break potential Deface overrides stores could have made.

Before

After

tvdeyen · 2017-04-04T09:45:33Z

backend/app/assets/stylesheets/spree/backend/plugins/_select2.scss

@@ -49,7 +49,7 @@
  }


Probably not caused by any change made in this PR I guess, but poking around I stumbled over a wrong background colour on disabled select dropdown arrows.

Before

.select2-arrow { ... b { background: transparent ...; ... } }

I would even prefer to only make the background-image !important, but to keep the changeset as small as possible I just changed the background color

After

tvdeyen · 2017-04-04T09:54:24Z

backend/app/assets/stylesheets/spree/backend/shared/_typography.scss

@@ -1,5 +1,10 @@
 // Base
 //--------------------------------------------------------------
+
+html {
+  font-size: $font-size-root;


Bootstrap removed this for accessibility reasons twbs/bootstrap@bd38a2a

We should follow their example.

I would like that, but I think we should do this in a separate PR. Removing this would move us from 13px to 16px (maybe), which is a really significant change, which I'm sure will cause some issues. I'd rather we look at that separately.

As long as we set $font-size-base to $body-font-size everything should be fine.

That changes the size of our rem, which I don't want to do. It's awkward that our base font size doesn't match a rem, making rem pretty useless if it doesn't match up with anything else on the page. If we're setting $font-size-base to a px value anyways we're not improving accessibility in any way.

I appreciate the argument that using the browser's default font size is a good thing. But if we aren't (and I think it's too big a change for this PR) the way bootstrap used to do it, and the way we're doing it here, by setting a size on the root element, is best.

tvdeyen · 2017-04-04T11:42:53Z

backend/app/views/spree/admin/shared/named_types/_form.html.erb

@@ -1,6 +1,6 @@
 <div data-hook="admin_named_type_form_fields" class="row">
  <div class="row">


These nested rows now break the layout

We should remove the nested row and change the columns size back to 4 as this is the value it was in our skeleton based UI.

Before

After

jhawthorn · 2017-04-04T21:29:59Z

@tvdeyen I've pulled in your changes other than changing the font size.

tvdeyen

🚢

jhawthorn added 7 commits April 3, 2017 13:06

Upgrade bootstrap to v4.0.0-alpha.6

235e2be

Fix breadcrumbs

e8bcfb2

Fix table spacing

8c05a26

Previously normalize.css set table's border-spacing to 0, bootstrap now uses reboot.css, which doesn't reset this property.

Replace boostrap's col-xs-N with col-N

107cd73

Bootstrap has removed col-xs-N in the latest alphas.

Disable animations on tooltips

a683779

This works around a bootstrap 4.0.0.alpha6 issue resulting in Error: Tooltip is transitioning We should be able to re-enable this for the next release if desired.

Fix colours on tooltips

b9cdead

Fix font sizes and padding

ad2d12c

jhawthorn added the changelog:solidus_backend Changes to the solidus_backend gem label Apr 3, 2017

jhawthorn requested a review from tvdeyen April 3, 2017 20:09

jhawthorn added the Code review needed label Apr 3, 2017

This was referenced Apr 3, 2017

Upgrade to Bootstrap 4.0.0.alpha6 #1776

Closed

Show focused border on currency selector #1813

Merged

Use bootstrap input-group for date range #1817

Merged

tvdeyen requested changes Apr 4, 2017

View reviewed changes

jhawthorn mentioned this pull request Apr 4, 2017

Redesign product details page in admin #1818

Closed

tvdeyen added 6 commits April 4, 2017 14:24

Fix disabled select2 arrow background color

736afed

Fixes input height by using bootstraps input line height

54f236f

Fixes order add product autocomplete width

c20ccc8

Fixes order shipments select width

93e4555

Fixes width of reimbursements reasons form

077e212

Add padding to variant select result image

4842050

tvdeyen approved these changes Apr 5, 2017

View reviewed changes

jhawthorn merged commit f6b660d into solidusio:master Apr 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Bootstrap 4.0.0.alpha6 #1816

Upgrade to Bootstrap 4.0.0.alpha6 #1816

jhawthorn commented Apr 3, 2017

tvdeyen left a comment

tvdeyen Apr 4, 2017

tvdeyen Apr 4, 2017

tvdeyen Apr 4, 2017

tvdeyen Apr 4, 2017

jhawthorn Apr 4, 2017 •

edited

Loading

tvdeyen Apr 4, 2017

tvdeyen Apr 4, 2017

tvdeyen Apr 4, 2017

jhawthorn Apr 4, 2017

tvdeyen Apr 4, 2017

jhawthorn Apr 4, 2017

tvdeyen Apr 5, 2017

tvdeyen Apr 4, 2017

jhawthorn commented Apr 4, 2017

tvdeyen left a comment

		@@ -1,6 +1,6 @@
		<div data-hook="admin_named_type_form_fields" class="row">
		<div class="row">

Upgrade to Bootstrap 4.0.0.alpha6 #1816

Upgrade to Bootstrap 4.0.0.alpha6 #1816

Conversation

jhawthorn commented Apr 3, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before

After

Choose a reason for hiding this comment

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before

After

jhawthorn Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before

After

Choose a reason for hiding this comment

Before

After

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

Before

After

jhawthorn commented Apr 4, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

jhawthorn Apr 4, 2017 •

edited

Loading