More Seven Theme issues: #1986434: New visual style for Seven
Problem/Motivation
We aim to tweaks the styling of the navigation listings to bring them more inline with the new Seven visual language.
We developed Proposal: A Style Guide for Seven. This issue aims to introduce the proposed styling for pagination in core.
To quote the rationale provided:
The Nav List is used on “hub” pages to provide an on-page navigation menu with help text. This component is not greatly changed from D7, with only minor typographic finessing and a rounding of the “fancy bullet” used to mark the list items, in keeping with the friendliness of the brand.
Proposed resolution
In #1945552: Add Nav List component we laid the ground work for this change.
Remaining tasks
- Make a patch
User interface changes
This is a style only change,
API changes
No API Changes
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#42 | rtl-admin-config-before.png | 289.01 KB | rteijeiro |
#42 | rtl-admin-config-after.jpeg | 257.52 KB | rteijeiro |
#42 | rtl-admin-reports-before.png | 90.81 KB | rteijeiro |
#42 | rtl-admin-reports-after.png | 93.78 KB | rteijeiro |
#41 | ie8-nosvg.png | 13.84 KB | echoz |
Comments
Comment #1
LewisNymanHere's the initial patch
Comment #3
yoroy CreditAttribution: yoroy commented#1: new-admin-list-style.patch queued for re-testing.
Comment #4
echoz CreditAttribution: echoz commentedLookin' good.
* The .png is misnamed in the rtl file ("list-icon-rtl.png" rather than "nav-list-icon-rtl.png")
* git apply reports trailing whitespace on the pngs.
Comment #5
LewisNymanThanks, I changed the class in the RTL file. Not sure about the whitespace in the png. I ran them through imageoptim....
Comment #6
echoz CreditAttribution: echoz commented#5 patch changed the wrong unrelated img ruleset. Here is that correction. Patch has the same git warning, but applies and stages fine. Implementation looks fine.
-------
2016875-nav-list-6.patch:26: trailing whitespace.
?PNG
2016875-nav-list-6.patch:35: trailing whitespace.
?PNG
warning: 2 lines add whitespace errors.
Comment #7
Bojhan CreditAttribution: Bojhan commentedCan we fix these whitespace errors?
The only thing I am concerned about is the fact that it vertically centers to the title. That looks a bit strange, but it looks totally fine without descriptions. Given that many content facing occasions there won't be descriptions. I am fine with leaving that, also with typographic changes it will look better.
Comment #8
echoz CreditAttribution: echoz commented@Bojhan, not sure what you mean about "vertically centers to the title". With desciptions, I would think this is desired, same as previously.
The images were not transparent, they were on white. Since they are on a non white background on the config page, I've re-done the images from seventy-eight's PSD (plus optimized). I was hoping the whitespace warnings would go away, but they are the same. Note they are warnings not errors.
Only change in this patch is transparent images.
Comment #9
LewisNymanThe icon looks good aligned with the title, @Bojhan if you want to we could add a variant class if a description exists?
Comment #10
yoroy CreditAttribution: yoroy commentedMiddle alignment of icon and title, with or without a description is the way to go. I don't think a variant class is needed.
Comment #11
echoz CreditAttribution: echoz commentedAdjustment for previously untested RTL
Comment #12
Bojhan CreditAttribution: Bojhan commented@echoz Nice work!
Comment #13
echoz CreditAttribution: echoz commentedRe-roll since #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute got committed.
Comment #14
rteijeiro CreditAttribution: rteijeiro commentedUpdated according to #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute.
Comment #15
rteijeiro CreditAttribution: rteijeiro commentedThe patch :(
Comment #16
LewisNymanThe last patch seems a little bit overreaching, should we make the changes here or as part of #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute ?
Comment #17
rteijeiro CreditAttribution: rteijeiro commentedYou are right @Lewis, I have included some crap from other patch, sorry :(
This is the right patch ;)
Comment #18
echoz CreditAttribution: echoz commentedHold on. First of all, the patches in #15 or #17 (which are identical, btw) leave out the new icons. They are changing the rtl attribute selector in seven's style.css to have quotes. But #2030925: quote rtl attribute selector has not landed yet. Whichever patch gets committed first (2030925 or this one), the other will need to be re-rolled. The patch that will work right now is #13, which I am resubmitting here. If #2030925: quote rtl attribute selector gets committed before it, I will re-roll this with changing I think only one needed "rtl".
Comment #19
rteijeiro CreditAttribution: rteijeiro commentedI agree with @echoz. Sorry for the noise :(
Patch applies well and seems to work well.
Comment #20
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #21
echoz CreditAttribution: echoz commentedre-roll since #2030925: quote rtl attribute selector landed
Comment #23
echoz CreditAttribution: echoz commented#21: 2016875-nav-list-21.patch queued for re-testing.
Comment #24
YesCT CreditAttribution: YesCT commentedrtl is different than in #11.
is that because the fix for rtl will be in another issue?
admin/config
no patch
no patch - rtl
patch
patch - rtl
admin/reports
no patch
patch
Comment #25
LewisNymanYep, we could of easily lost some RTL styling in the past week.
Comment #26
YesCT CreditAttribution: YesCT commentedtagging for d8mi frontend people.
Comment #27
echoz CreditAttribution: echoz commentedIt was a specificity issue that my browser cache fooled me into thinking I had fixed.
Comment #28
echoz CreditAttribution: echoz commentedThe column css was screwed up (previous to this issue) and issue only showed up on RTL. Fixed.
patched LTR
patched RTL
Comment #30
echoz CreditAttribution: echoz commented#28: 2016875-nav-list-28.patch queued for re-testing.
Comment #31
echoz CreditAttribution: echoz commentedRemoved indentatation on "Hide description" display, as per #2037525: 'Hide description' layout bug
Comment #32
echoz CreditAttribution: echoz commented…and a screenshot
Comment #34
echoz CreditAttribution: echoz commented#31: 2016875-nav-list-31.patch queued for re-testing.
Comment #35
echoz CreditAttribution: echoz commentedThis still applies and should be easy to review (admin/config and admin/reports), can we move this along?
Comment #36
YesCT CreditAttribution: YesCT commentedI swear I think someone at NYCCamp made all the before and after screenshots and double checked this...
Comment #37
Bojhan CreditAttribution: Bojhan commentedThis smells like RTBC
Comment #38
echoz CreditAttribution: echoz commentedI remember somewhere it was said we should leverage the icons from ry5n's Libricons #2032773: Use Libricons (icon font) in Seven, consider using it more broadly in core for the icons used here. In #1963886: Use HiDPI icons in the toolbar only the icons used in toolbar were added in misc/icons, which included chevron-disc up + down but not left + right, which are part of Libricons and the ones used here in the admin nav lists.
This new patch is the same as #31 patch, which still applys and was ready for rtbc, plus uses the svg (w/ Modernizer png fallback) from misc/icons.
It adds these icons, in the right color, at misc/icons:
chevron-disc-left.png
chevron-disc-left.svg
chevron-disc-right.png
chevron-disc-right.svg
It does not add the pngs (created from seventy-eight's PSDs) that had been used up to the #31 patch. If we decide not to use the svg / png from misc/icons, we can use the #31 patch.
Using svg icons from misc/icons
Comment #39
echoz CreditAttribution: echoz commentedtagging
Comment #40
rteijeiro CreditAttribution: rteijeiro commentedPatch applies well (except for the errors regarding existing icon files) and looks really good.
Maybe it's a RTBC?
Comment #41
echoz CreditAttribution: echoz commentedThe git warnings (not errors) have always been there for the pngs.
I wanted to hear if nav-list is a candidate for switching to use svg icons, which I assume is true and what this last patch #38 does. Here's a screenshot showing that ie8 gets the Modernizer png fallback.
RTL should be tested since this change (admin/config and admin/reports).
Comment #42
rteijeiro CreditAttribution: rteijeiro commentedIt looks ok for me. Tested for RTL and it seems to fix a bug in admin/config page. Take a look at the screenshots:
admin/config BEFORE:
admin/config AFTER:
admin/reports BEFORE:
admin/reports AFTER:
Well done echoz!!
Maybe RTBC?
Comment #43
echoz CreditAttribution: echoz commentedThanks for the RTL testing!
Correct, I discovered and fixed that column bug in #28 that existed previous to this issue but only showed up on RTL.
Comment #44
kfritscheJust tested it again with LTR and RTL.
Still works - RTCB
No reroll needed.
Comment #45
yoroy CreditAttribution: yoroy commentedYes, this is ready to get in!
Comment #46
webchickThat looks much better!
Committed and pushed to 8.x. Thanks!
Comment #47.0
(not verified) CreditAttribution: commentedadded related Use Libricons meta issue, removed 1849712