Problem/Motivation
New views grid look a little out of style in Bartik. We need to add some styling cleanup and responsive support for views-grids so they look/work properly.
Proposed resolution
Cleaning up the CSS
Remaining tasks
Manual review
User interface changes
Grid view CSS
API changes
None
Original report
The patch at #1903746-44: Replace the views grid table template with one using divs makes the new views grid look a little janky in Bartik. We need to add some styling cleanup and responsive support for views-grids so they look/work properly.
Comment | File | Size | Author |
---|---|---|---|
#87 | interdiff.txt | 839 bytes | lauriii |
#87 | 2031447-87.patch | 1.58 KB | lauriii |
#84 | after.png | 141.51 KB | anmolgoyal74 |
#84 | before.png | 140.79 KB | anmolgoyal74 |
#81 | afterPatch_mobileView2.png | 23.04 KB | Ruchi Joshi |
Issue fork drupal-2031447
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
markhalliwellPatch
Comment #2
markhalliwellHorizontal:
Vertical:
Comment #3
dawehnerJust in general they are looking great!
Comment #4
markhalliwellRemoved the first/last class selectors since they're no longer provided by the plugin
Comment #5
markhalliwellNeeds to be visually tested with patch from #1903746: Replace the views grid table template with one using divs:
http://simplytest.me/project/drupal/8.x?patch[]=https://drupal.org/files/1903746-complete.patch&patch[]=https://drupal.org/files/drupal-add-new-views-grid-2031447-4.patch
Comment #6
markhalliwell#1903746: Replace the views grid table template with one using divs is now in core!
Need to test with only this issues patch now:
http://simplytest.me/project/drupal/8.x?patch[]=https://drupal.org/files/drupal-add-new-views-grid-2031447-4.patch
Comment #7
joelpittetLooks good, just a couple CSS nitpiks and I say ship it:)
0 alone will suffice.
Comment #8
markhalliwellFixed very minor nitpick in #7.
Comment #9
joelpittetThis looks good to me and is fairly small bit of CSS to boot.
RTBC+1.
Comment #10
alexpottAssigning to John Albin for a css review...
Comment #11
xjm#8: drupal-add-new-views-grid-2031447-8.patch queued for re-testing.
Comment #12
webchickI think alexpott meant to move this down to "needs review."
I don't know that John Albin Himself™ needs to review this, but what we need is something a bit more firm like "I inspected the patch line by line, can confirm the approach taken here is the best one, checked it against the CSS/markup standards, and tested it in X browsers, and it looks good to me."
Comment #13
brahmjeet789 CreditAttribution: brahmjeet789 commentedI have applied the patch #8 its working fine for my machine .
Comment #14
klonos@brahmjeet789: please see what Angie said back in #12 would qualify to set this issue to RTBC.
Comment #15
jibran8: drupal-add-new-views-grid-2031447-8.patch queued for re-testing.
Comment #16
joelpittetComment #17
LewisNyman CreditAttribution: LewisNyman commentedIs there a good reason not to use mobile first media queries here?
I can see few situations where we're adding styling just to undo it for first/last children.
Can I suggest the adjacent sibling selector instead? It would reduce the code required a lot.
Comment #18
LewisNyman CreditAttribution: LewisNyman commentedComment #19
dawehnerNot that I have any clue about css, but
width: 100% !important
really looks desperate, is this needed?
@LewisNyman
It would be great if you can find someone at drupalcon to work on this problem.
Comment #20
emma.mariaOn it :)
Comment #21
lauriiiAnswer for #17/1 and #19: We need to use
width: 100% !important
because we have width 50% as inline CSS in the.views-col
. If we would want something better, we need to change Views grid to use something else than inline CSS (classes?).Comment #22
joelpittetThanks @lauriii for the patch and figuring out why we are using !important. Would you mind opening up an issue to move that inline styles to css classes? i don't think this needs to be dependant on that issue and the !important can go in, but it would be nice to also look at that approach on it's own... it may be hard with different grid sizes to come up with a set of class names that aren't bloating the views CSS but I do believe worth a try. What do you think?
Maybe 1-12 columns and anything more than that could dealt with custom CSS? Or an option to do inline styles with tokens based on the columns may be worth a look.
Comment #23
markhalliwellNo, the
!important
is necessary because the views grid style plugin has an "Automatic width" setting which adds the dynamic width via PHP: http://cgit.drupalcode.org/drupal/tree/core/modules/views/views.theme.in...A bug I just realized is that this would apply to grids that do not use that setting. It should also really reset to it's initial width (determined by element type), not just 100%.
So perhaps it should really be the following instead:
Also, I know I created this (a long time ago), but it really should be mobile first with min breakpoints, not max. Cannot remember if it's because bartik wasn't mobile first (at the time) and I was just following existing code.
Comment #24
lauriiiIf I understood right, @joelpittet is talking about "Automatic width" setting in this comment. I opened new issue for that: #2294153: Use classes instead of inline CSS in Views automatic width option
@Mark Carver thank you for your input, I didn't know
width: initial
exists. With that setting it is possible to make it 100% mobile-first. I can keep working on this one.Comment #25
markhalliwellSorry, that was a typo. It should be
inherit
, notinitial
(which isn't supported by IE).Comment #26
lauriiiWhat do you think could we use
min-width: 100%
andmin-width: 0
to override inline CSS? Then we wouldn't need any!important
or non mobile first media queries.Comment #27
markhalliwellYes, that could work.
Comment #28
teroelonen CreditAttribution: teroelonen commentedAdded the min-width fix and seems to be working real well. Browser support for min-width.
Mobile screenshot:
Mobile horizontal screenshot:
Tablet screenshot:
Desktop screenshot:
Comment #29
emma.mariaComment #30
emma.mariaThis issue needs more work than just adding the code from the previous patch again.
Looking at the screenshots in #26. We have acquired a border bottom on the bottom of every Node teaser. This style is applied as follows...
By default this is great for the homepage view, where you get the standard view of most recent content, a nice border underneath each teaser. But, no matter where or how the teasers are displayed you get a border bottom which in my eyes is incorrect. If the user has views knowledge (and views is in core now) and wants to display a grid, or a teaser in the sidebar, triptych/tertiary region they will be faced with a bottom border.
So as part of using the ace work above by @Mark Carver we need to tackle the issue I raised above too.
Comment #31
DickJohnson CreditAttribution: DickJohnson commentedAll the patches have been written before the SMACSS split, so reroll is needed anyways. Starting with that one.
Comment #32
emma.mariaThis should not be set to Needs Review, the work to fix the design for grids needs to be completed too.
The styling for the lines under each teaser needs to be reworked so this doesn't happen...
We are aiming for the screenshots in #2
Comment #33
DickJohnson CreditAttribution: DickJohnson commentedThe need review was just to get feedback from bot. Forgot to assign it to myself so assigning now.
Comment #34
DickJohnson CreditAttribution: DickJohnson commentedComment #35
LewisNyman CreditAttribution: LewisNyman commentedThese properties aren't indented. Also we don't need to include -moz- or -webkit- prefixes anymore
Min-width: 0? What does this do? You can't have less than 0 :P
Only add blank links above comments please.
Comment #36
DickJohnson CreditAttribution: DickJohnson commentedStarted to work on cleanup. I'm travelling to another part of country and changing computer so putting patch here to see test bot and to keep it safe. No review needed, will continue in few hours.
Comment #37
DickJohnson CreditAttribution: DickJohnson commentedFixed some coding standard issues.
@lewisnyman asked in #35 why min-width: 0; is there. It's overriding min-width: 100%; that we're using to override inline width: 33.33333% and making the grid of 3 to actually work. There is a better solution for that and I'll take a look on it later today.
Comment #38
DickJohnson CreditAttribution: DickJohnson commentedBefore the breakpoint was written to 520px, I resized it to 540px but I think it's still too thin. As a attacment screenshot of 541px.
I think we should change the breakpoint to somewhere like 620px of 640px. As an attachment example of 640px.
Comment #39
DickJohnson CreditAttribution: DickJohnson commentedNow I think it's starting to be in relatively good shape.
Pad:
Desktop:
Comment #40
DickJohnson CreditAttribution: DickJohnson commentedUnassigning. I didn't fix the min-width: 0; mentioned by @lewisnyman previously as I didn't find a good solution for that.
Comment #41
emma.mariaComment #42
mherchelRerolled. I also removed some extra linebreaks.
Can probably use some additional visual regression testing.
Comment #43
mherchelPrior patch had a weird issue (encoding?). New patch attached. Still needs visual review.
Comment #44
joelpittetHa, cross posted with you, thanks for cleaning that up:)
These line removal and dropping /*LTR*/ changes shouldn't be made.
Comment #45
joelpittetIs there a class we could target here? Would be nicer than trying to target a header at a specific level.
Comment #46
LewisNyman CreditAttribution: LewisNyman commentedhmm, node__title feels like a better class to use here.
Comment #47
emma.mariaComment #48
jorgeegomez CreditAttribution: jorgeegomez commentedCleaned up mherchel's patch in #43, according to joelpittet and LewisNyman comments on #44 and #46
Comment #49
joelpittetAwesome thanks @jorgeegomez, just needs manual screenshots.
Super minor nitpick:
Needs a period ending the sentence.
Comment #50
stephenrodrigo@yahoo.com CreditAttribution: stephenrodrigo@yahoo.com commentedComment #51
stephenrodrigo@yahoo.com CreditAttribution: stephenrodrigo@yahoo.com commented@joelpittet - I have attached screenshot from Chrome 40.0.2214.115.
In vertical grid view column percentage value breaking the structure because of the border.
Needs to add box-sizing: border-box to .views-view-grid.vertical .views-col
Comment #52
stephenrodrigo@yahoo.com CreditAttribution: stephenrodrigo@yahoo.com commentedComment #53
stephenrodrigo@yahoo.com CreditAttribution: stephenrodrigo@yahoo.com commentedI have actually patch the vertical grid view column structure.
Comment #54
stephenrodrigo@yahoo.com CreditAttribution: stephenrodrigo@yahoo.com commentedI have missed the last curly bracket. It is fixed in this patch.
Comment #55
emma.mariaI have tested the Views grid styling with 10 nodes at different screen widths.
Everything looks great apart from one thing.
Above 640px the padding to space out the teasers horizontally (which is applied to
.views-view-grid .node--view-mode-teaser
) causes the teasers to not be flush with the edges of the view container....We need to move the padding from that level in the markup to the
views-col
level.Code improvement suggestions
Remove the padding from this level of the markup.
Add padding-right: 15px; and box-sizing: border-box; to this section.
Keep the current last-child styling but remove the first-child as it is not needed.
Comment #56
pakmanlhWorkin on it.
Comment #57
pakmanlhI attached a new patch following the #55 instructions.
Comment #58
emma.mariaRemoving the padding at the beginning and end of the row makes the view look much better.
However of course adding padding to all rows apart from the last means the last row is a bit wider than the others.
I am not so keen on overriding the widths of the columns which are set by an inline style from Views by force. But 25% seems to be the only value Views provides as an inline style for grids, so it might be possible that will never change? For any amount less than 4 items the teasers just stack.
Also one more design improvement I missed.
The original grid styling had a border at the very bottom row, we should keep that in the way we use the borders now.
Please remove this code from the patch:
Comment #59
pakmanlhUpdating with some thoughts. After a review of the grid views options I think we might assume that the user will uncheck the "automatic width" option to use the Bartik (or theme) configuration styles because the advice message is clear enough.
After that we could prepare a finit number of col-n (col1, col-2, ...) styles, until 16 or 24 for example, and then use also the cols-n selector to determine how many columns have the grid and apply the respective percentages.
What do you think? :)
Comment #60
emma.mariaComment #61
dshesq CreditAttribution: dshesq as a volunteer commentedAfter trying to get a 200x200 image to display using Views > Grid, the result only produces a single column. Using the same set up in 7 gets a display of 4 images across. Before I start to look at the code, any suggestions?
Comment #62
lauriiiComment #63
Sabreena CreditAttribution: Sabreena at Axelerant commentedComment #64
Sabreena CreditAttribution: Sabreena at Axelerant commentedComment #74
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedRe-rolled the patch against the 9.1.x
Comment #75
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedWorked on the CSS-lint issue and https://www.drupal.org/project/drupal/issues/2031447#comment-9836693.
Attached screenshot for the references.
Comment #76
Ruchi Joshi CreditAttribution: Ruchi Joshi at Srijan | A Material+ Company for Drupal India Association commented@komalkolekar- On applying patch, its working fine for "Horizontal" grid view but not working for both Vertical view and Mobile version. Screenshots are attached.
Comment #77
Santosh_Verma CreditAttribution: Santosh_Verma at Srijan | A Material+ Company for Drupal India Association commentedI have worked on this issue, now its working fine for Horizontal and Vertical view as well for Mobile and I pad.
I am attaching the screenshot for reference.
Comment #78
Ruchi Joshi CreditAttribution: Ruchi Joshi at Srijan | A Material+ Company for Drupal India Association commentedMoving it to "Needs work" for patch re-roll
Comment #79
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedpatch #77 Applied successfully.
Work on the CSS lint issues.
Comment #80
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedForget to add the interdiff.
Comment #81
Ruchi Joshi CreditAttribution: Ruchi Joshi at Srijan | A Material+ Company for Drupal India Association commentedTested issue with patch#79. New Grid view is working fine for Bartik theme. Screenshots are attached. @markcarver +1 for RTBC.
Steps:
1. Visit /admin/structure/views
2. Click on "Add view" button
3. Create a view page filling- view name and click on create a page.
4. Then click on "Save and edit" button
5. Create page with same url alias which created during creation of view.
6. Click on "Format | Unformatted list" link and click on Grid radio button.
7. Also click on "Format | Settings" link and switch between "Alignment" as Horizontal or Vertical for different views of same page.
Comment #83
tanubansal CreditAttribution: tanubansal at Salsa Digital commented#79 works for me at 9.1
RTBC + 1
Comment #84
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedWorking for 9.2.x as well. Marking it as RTBC.
Comment #86
alexpottCrediting people who provided patches and rerolls and both before and after screenshots. Thanks to everyone for all the work on moving this issue forward over the years.
Comment #87
lauriiiI manually tested both vertical and horizontal grids quite extensively and everything seems to work as expected. The code looks very good. Here's interdiff for few minor changes that we should still make.
Comment #88
alexpottCommitted e543925 and pushed to 9.2.x. Thanks!