Add bootstrap v4.0.0-alpha.2 into the admin #955
Conversation
|
The small misalignment of things everywhere is a no-go to me. I'd prefer to see us importing things one component at a time. What was your thought on that? |
We spoke IRL, most of our misalignments are going to be due to bootstrap's reboot and typography, and it isn't much use to include bootstrap without those. I'm going to try to fix up as many issues as possible before merging and have created a checklist of issues. |
| bootstrap4 styles. If you have a custom admin page with pagination you can | ||
| use this style with the following. | ||
|
|
||
| <%= paginate @collection, theme: "admin" %> |
There was a problem hiding this comment.
This theme should be called solidus
There was a problem hiding this comment.
Done. I went with "solidus_admin"
jhawthorn
force-pushed
the
admin_bootstrap4
branch
from
77508ba to
3d9658b
Compare
|
I think this is good to go 🚢 I spent a while browsing through the admin and can no longer find anything noticeably broken. Spacing has of course changed slightly in a few places, but nothing I've found looks out of place. |
Previous to the addition of bootstrap, our :before and :after pseudo-elements were not box-sizing: border-box, which their spacing depended on. This adds padding: 0 to the no-text versions of the icons, which removed the right padding that is needed for the icons when combined with a button.
Previously we included just this one scss file from bootstrap to do our media components.
Previously there were some minor layout issues due to both grids claiming ownership of the "container" and "row" classes. Skeleton sets container to a fixed width 960px, where bootstrap by default uses a max-width of 940px. Bootstrap also provides styles for "row", as part of its grid system. This disables the generation of grid classes in bootstrap to avoid the mixed styling of "row". To make use of the bootstrap grids we will be using mixins. This changes the max-size of the container in bootstrap to 960px, so that containers use the existing size defined by skeleton.
jhawthorn
force-pushed
the
admin_bootstrap4
branch
from
ebf75e6 to
bf2030b
Compare
Also adds tether.js, which is required by bootstrap's js.
jhawthorn
force-pushed
the
admin_bootstrap4
branch
from
bf2030b to
738ae1c
Compare
|
I replaced the a-la-carte individual JS files with the single |
This is a more modern approach to grids, which we can use because we require modern browsers in the admin.
|
Why not just deliver the files, but not require them. Then we could have a slim JS file for "default" Solidus and extension developers are still able to require components they need. For instance, the Also, I thought we agreed on porting one component at a time, so we basically don't need any JS component right know? Am I missing something? /c @graygilmore |
|
I prefer including the entire bootstrap.js file. I suspect it will be a point of confusion for users if some bootstrap features don't work (the bootstrap docs aren't as clear as they could be about needing to include js for the IMO if we were going to sweat a couple extra KB, we wouldn't be using bootstrap in the first place. |
|
As discussed in slack we go with the whole file 👍 |
already!Add bootstrap v4.0.0-alpha.2 into the admin
|
YOLO! |
|
Thanks @jhawthorn didn't get a chance to chime in before this got merged but 👍 great work! |
The goal of this is to get bootstrap included into the admin with minimal changes. After this is merged we can begin replacing components as we see fit.
Rather than including the bootstrap-sass gem, I have simply added the scss into
vendor/bundle. This is to avoid tying the version of bootstrap we use in the admin with the version stores use in their frontend, and is how we should be handing all admin assets from now on.After including bootstrap, the only major issue I noticed was broken pagination. Rather than fighting with the bootstrap classes, I chose to remove our pagination component in favour of the one included with bootstrap. So the styles of that have changed (and probably need their colours fixed).
Issues:
<th>is now left aligned instead of centered<legend>s at the top of fieldsets have lost their line separator