There's a strong argument to be made for applying box-sizing: border-box
.
- It's easier to write fluid layouts, you can do crazy things like
width: 80%; padding: 0 10px;
- Contrib modules can rely on it being applied, which means they don't need to add inner/wrapper divs on things to have reliable styling.
The downside is this will affect every width set by in core and contrib. We implemented this on the D7 version of Bluecheese and came across a few bugs with contrib module CSS. If contrib modules want a responsive admin interface in D8 they are probably going to have to rewrite all their CSS anyway so this probably isn't that big a deal.
Things left to do:
- Move the box-sizing "hack" into system.theme.css.
- Review the new patch again.
- Take screenshots to make sure we don't break anything.
- Move into classy?
Comment | File | Size | Author |
---|---|---|---|
#77 | apply_box_sizing-2124251-77.patch | 906 bytes | alvar0hurtad0 |
#72 | apply_box_sizing-2124251-72.patch | 444 bytes | meenakshi.r |
#66 | apply_box_sizing-2124251-66.patch | 442 bytes | junaidmasoodi |
#62 | apply_box_sizing-2124251-62.patch | 574 bytes | junaidmasoodi |
#59 | manage_form.png | 42.08 KB | ellizard |
Comments
Comment #0.0
LewisNyman CreditAttribution: LewisNyman commentedthat
Comment #1
LewisNyman CreditAttribution: LewisNyman commentedNo chance of doing this so late in the release cycle
Comment #2
Bojhan CreditAttribution: Bojhan commentedIs this something we can do in 8.1? If you really want it, we can only do it now (before beta).
Comment #3
sqndr CreditAttribution: sqndr commentedAdding this would be a lovely idea. It might be a little to late. If we were to change it now, we need some very good manual testing. There are however good testing frameworks, for taking screenshot of all the pages. Maybe this would be something cool to do at the camp/con/...? Write a patch and ask lots of people to test it. It's a good way as well to introduce them to applying patches and make comments?
Comment #4
LewisNyman CreditAttribution: LewisNyman commentedOk, If we have a small window before beta than maybe we can give this a go at frontend united or another camp.
Comment #5
sqndr CreditAttribution: sqndr commentedhttp://www.youtube.com/watch?v=83I9El6C47A (no animated gif this time).
It's something that needs good testing. Let's do this!
Comment #6
sqndr CreditAttribution: sqndr commentedHere we go with the first patch ...
Comment #7
sqndr CreditAttribution: sqndr commentedI've created a new patch. No interdiff, because it didn't make much sense. I removed all of the
box-sizing: border-box
, including the vendor prefixes. It needs a review, and some proper testing.Comment #8
sqndr CreditAttribution: sqndr commentedComment #9
droplet CreditAttribution: droplet commentedIt should includes a rule for before/after generated content.
http://css-tricks.com/inheriting-box-sizing-probably-slightly-better-bes...
Comment #10
sqndr CreditAttribution: sqndr commentedAll right.
Comment #11
LewisNyman CreditAttribution: LewisNyman commentedIf we implement the change in #9 then I am happy to give this a quick manual test + screenshots.
Comment #12
sqndr CreditAttribution: sqndr commentedHere's a new patch. Meanwhile, all of the Seven css has moved, so I didn't include an interdiff.
Comment #13
Wim LeersIs it really safe to lose the vendor prefixes?
Comment #14
sqndr CreditAttribution: sqndr commented@Wim
box-sizing:
now has native support in those browsers. See the related issue for more information #1664268: Drop some browser specific prefixes.Comment #15
sqndr CreditAttribution: sqndr commentedComment #16
sqndr CreditAttribution: sqndr commentedComment #17
Wim LeersExcellent :) I suspected that, but couldn't find confirmation easily on http://caniuse.com. Thanks!
Comment #18
LewisNyman CreditAttribution: LewisNyman commentedAll we need now is for someone to double check a load of pages in the Seven theme to make sure we haven't broken anything visually. View's UI is a good place to check.
Comment #19
sqndr CreditAttribution: sqndr commentedIf someone wants to start with this, go ahead. Otherwise I'll try to write an automated test for this.
Comment #20
sqndr CreditAttribution: sqndr commentedWe need a review for this issue :)
Comment #21
LewisNyman CreditAttribution: LewisNyman commentedAw sorry I needs a reroll. I will reroll and test
Comment #24
LewisNyman CreditAttribution: LewisNyman commentedI found a regression on table drag handles:
I also found box-sizing declaration in the following admin.css files:
core/modules/block/css/block.admin.css
core/modules/ckeditor/css/ckeditor.admin.css
core/modules/node/css/node.module.css
core/modules/system/css/system.admin.css
core/modules/views_ui/css/views_ui.admin.css
That is appears in so many places is a good sign in terms of any UI regressions, I feel like we should remove these values and move the declaration into system.admin.css like we did in #2298821: Move generic layout styling into system.admin.css
Comment #25
tim.plunkettFixing tags.
Comment #26
jerrylow CreditAttribution: jerrylow commentedVerifying and grabbing some screens.
Comment #27
jerrylow CreditAttribution: jerrylow commentedThere's a new /*LTR*/ added to this line now. I'm adding it to the updated patch here. I've manually gone through all the pages and settings. Attaching a bunch of screens of all the pages working (desktop testing only - thus far).
Here are some other regressions I've found:
Besides the drag icon that Lewis have found I've noticed these drag icons in the search page settings is not affected:
It does break when the other arrows are fixed, but if we move * { box-sizing: border-box; } to system.admin.css it should fix it - here's a screenshot of it inline:
The arrows are centered but looks like the text is not.
Updated patch for the tabs.css change as well as fixing the regression styles. Haven't moved the admin settings to system.admin.css yet.
Comment #28
brahmjeet789 CreditAttribution: brahmjeet789 commentedComment #29
brahmjeet789 CreditAttribution: brahmjeet789 commentedComment #30
sqndr CreditAttribution: sqndr commentedWhat work still has to be done for this issue?
Comment #31
LewisNyman CreditAttribution: LewisNyman commented@sqndr I think we just need to verify the regressions in #27 are fixed. And maybe another click around just to make sure we haven't missed anything else
Comment #32
sqndr CreditAttribution: sqndr commentedDoes this mean there's still work to do as well?
Comment #33
LewisNyman CreditAttribution: LewisNyman commentedIt makes sense to move it to system.admin.css.
Comment #34
jerrylow CreditAttribution: jerrylow commentedHere's an updated patch of moving those border-box to system.admin.css with screenshots.
Comment #35
jerrylow CreditAttribution: jerrylow commentedComment #36
nitishchopra CreditAttribution: nitishchopra commentedThe above patch looked fine for me.
Comment #37
idebr CreditAttribution: idebr commentedWith manual testing I found a few changes that should be fixed:
Table drag handle width is smaller
Warning/error icon column is smaller
Screenshots from themes are smaller
Buttons in the CKEditor are smaller
Vertical tabs summary now has a grey border on the right
Comment #38
sqndr CreditAttribution: sqndr commented@jerrylow: Any progress here?
Comment #39
jerrylow CreditAttribution: jerrylow commented@idebr @sqndr - I just did a fresh rebuild, applied the patch and clear cached. Can't see any of the issues mentioned about. Screenshots of the pages attached. I've updated the patch since dev has changed.
Comment #40
jerrylow CreditAttribution: jerrylow commentedComment #41
sqndr CreditAttribution: sqndr commentedComment #42
Karmen CreditAttribution: Karmen commentedI've tested these paths and works for me.
Comment #43
alexpottAFAICS this is only changing the box model on admin pages but it is changing CSS that might not be on an admin page - for example dialog and quickedit.
Comment #44
LewisNyman CreditAttribution: LewisNyman at Wunder commentedThat's true, I think we would have to be consistent here across all themes if modules are relying on this when they set width. Either we don't set this at all and let it be set on a per-component level, or we set this globally across all themes. We could put this in system.theme.css, with an idea of moving it into Classy when the time comes.
Comment #45
sqndr CreditAttribution: sqndr commented+1 for that!
(Bootstrap and Foundation use this globally as well, instead of on a per component base).
Comment #46
Manjit.SinghComment #47
Manjit.Singhrerolling a patch
Comment #48
Manjit.SinghComment #50
Manjit.SinghComment #51
samiullah CreditAttribution: samiullah commentedComment #52
samiullah CreditAttribution: samiullah at Srijan | A Material+ Company commentedAfter applying latest patch above "box-sizing: border-box" is getting applied globally.
Manually tested on the following screens:
Comment #53
alexpott#43, #44, #45 don't look as though they've been addressed by the most recent patch.
Comment #54
alexpottComment #55
sqndr CreditAttribution: sqndr commented@Lewis:
What do you mean by that?
Comment #56
sqndr CreditAttribution: sqndr commentedComment #57
sqndr CreditAttribution: sqndr commentedComment #58
deepakaryan1988Removing sprint weekend tag!!
As suggested by @YesCT
Comment #59
ellizard CreditAttribution: ellizard commentedApply 'box-sizing: border-box' globally to all element
Comment #61
junaidmasoodi CreditAttribution: junaidmasoodi as a volunteer and commentedComment #62
junaidmasoodi CreditAttribution: junaidmasoodi as a volunteer and commentedApplying box-sizing: border-box; globally
This reset gives us more flexibility than its predecessors — we can use
content-box
orpadding-box
(where supported) at will, without worrying about a universal selector overriding our CSS.Comment #63
junaidmasoodi CreditAttribution: junaidmasoodi as a volunteer and commentedComment #64
sqndr CreditAttribution: sqndr commentedI don't think we need to add vendor prefixes again. Browser support this native now.
Comment #65
junaidmasoodi CreditAttribution: junaidmasoodi as a volunteer and commentedComment #66
junaidmasoodi CreditAttribution: junaidmasoodi as a volunteer and commentedPatch re-submitted with removal of vendor prefixes
Comment #67
Manjit.Singhlooks good to me !! changing this to RTBC :)
Comment #68
sqndr CreditAttribution: sqndr commentedThanks everyone for working in on this issue. When look at the latest patch I have some concerns …
#62 (and #66) is basically the work I did all the way back in #12. We had to work on this issue to find regressions and fix those. We also had to move all the
box-sizing: border-box;
properties from all other elements, since we now add it globally. Also, we had to addcontent-box;
to elements that require a different box-sizing calculation.Since we're now all the way back to were we started, this feels odd to me. I think we need another review and good screenshots to show that everything is fixed now. I'm adding Needs Review back.
Comment #69
LewisNyman CreditAttribution: LewisNyman at Wunder commentedComment #70
Manjit.Singhspells of box-sizing is not correct in #66 patch.
Comment #71
sqndr CreditAttribution: sqndr commentedAnd there should be a space after the * here.
Comment #72
meenakshi.r CreditAttribution: meenakshi.r commentedChanges as suggested #70 & #71. Please verify.
Comment #73
sqndr CreditAttribution: sqndr commentedPlease read #68 and provide screenshots to verify the regressions are fixed.
Comment #74
meenakshi.r CreditAttribution: meenakshi.r commentedComment #76
Shamsher_Alam CreditAttribution: Shamsher_Alam commentedComment #77
alvar0hurtad0@Shamsher_Alam still working?
this is my approach, I've created a new css in classy because this issue #2566597: [Mega patch] Move system *.theme.css files to Classy
Comment #78
sqndr CreditAttribution: sqndr at Randstad Digital commentedI don't think this can still go in, because this will drastically change the styling on all pages and all css elements (since the use of *). I feel like this should be postponed.
Comment #79
LewisNyman CreditAttribution: LewisNyman at Wunder commentedYou are right, it also feels like we need to reduce the scope of this as we make this change to Classy and Stable.