Replace these icons with the Libricon SVGs and use CSS to apply the image instead of an inline <img>
tag
See: #1356602: [META] Clean up icons in misc
Remaining Tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
(Done-Comment #99, needs review) Draft a change record for the API changes | Instructions | ||
Update the patch to incorporate feedback from reviews (include an interdiff) | Instructions |
Before and after screenshots:
This issue improving the arrow display--using SVGs in place of PNGs, which takes care of poor quality on high-quality displays. Aside from a slight change in the sizing of the arrows, the before and after should appear to be the same. Please see comment #97 to see all the screen images.
Seven (Administrative Theme)
Up Arrow Before
Up Arrow After
Down Arrow Before
Down Arrow After
Beta phase evaluation
Issue category | Task because it's code cleanup |
---|---|
Unfrozen changes | Unfrozen because it only changes HTML/CSS |
Comment | File | Size | Author |
---|---|---|---|
#127 | replace_arrow_asc_and-2405057-127.patch | 11.87 KB | nlisgo |
#126 | Screenshot 2015-09-19 02.29.04.jpg | 720.62 KB | LewisNyman |
#125 | replace_arrow_asc_and-2405057-125.patch | 6.68 KB | madhavvyas |
#124 | replace_arrow_asc_and-2405057-124.patch | 5.22 KB | madhavvyas |
#121 | replace_arrow_asc_and-2405057-121.patch | 4.59 KB | madhavvyas |
Comments
Comment #1
ctraltdel CreditAttribution: ctraltdel commentedI replaced the arrow-asc.png and arrow-desc.png files in /core/misc with /core/misc/icons/bebebe/twistie-down.svg and /core/misc/icons/bebebe/twistie-up.svg. Modified template_preprocess_tablesort_indicator in /core/includes/theme.inc to set the 'arrow_asc' and 'arrow_desc' values in $variables to the new icons.
Note that in the default admin theme, Seven, these values are overridden to point to $theme_path . '/images/arrow-asc.png' and $theme_path . '/images/arrow-desc.png.' Based on https://www.drupal.org/node/2032773 it looks like that would be a separate issue that has already been discussed.
Comment #2
cilefen CreditAttribution: cilefen commented@ctraltdel: Could you please put before and after screenshots in the issue summary?
Comment #3
ctraltdel CreditAttribution: ctraltdel commentedComment #4
cilefen CreditAttribution: cilefen commentedComment #5
davidhernandezIn the screenshots the arrows are slightly smaller and they end up higher. The size doesn't matter, but is there any CSS attached to these that needs to be adjusted?
Comment #6
cilefen CreditAttribution: cilefen commentedThis looks good in classy as well. I am wondering why Seven didn't go with these. @ctraltdel, maybe you could discuss on that related issue if it was just overlooked.
Comment #7
cilefen CreditAttribution: cilefen commented@davidhernandez Good point. I should have waited for you first!
Comment #8
cilefen CreditAttribution: cilefen commentedSince these are SVGs, I assume we can just make them bigger.
Comment #9
davidhernandezI'm not actually seeing any CSS attached to them, so if that is the case I would leave it be. I wouldn't add extra CSS unless Lewis thinks moving them is important.
Comment #10
LewisNymanI think the only visual problem is the position of the arrow but I don't want to add CSS just to make Stark or Classy look good. I think we need to get a screenshot of Bartik to make sure that still looks ok?
Comment #11
cilefen CreditAttribution: cilefen commentedScreenshots in IS.
Comment #12
davidhernandezI just talked to emmamaria in IRC and she would like to fix the alignment in Bartik. It should only be a small bit of CSS. Add it to table.css in Bartik's components folder.
Comment #13
ctraltdel CreditAttribution: ctraltdel commentedI updated the table.css stylesheet in Bartik to align the images.
Comment #14
ctraltdel CreditAttribution: ctraltdel commentedComment #15
LewisNymanWe don't need so many elements here. table and tr are redundant because you can only have th within those elements anyway.
I would suggest: th img
We always include leading 0's before decimal points in CSS. Also why have padding for bottom and margin for left? Maybe we can combine them into one property?
Comment #16
ctraltdel CreditAttribution: ctraltdel commentedI made the suggested css updates. The end result has not changed from previous screenshots.
Comment #17
ctraltdel CreditAttribution: ctraltdel commentedComment #18
davidhernandezJust a quick FYI. When only changing part of a patch, it is good to upload an interdiff. It makes reviewing a little easier. https://www.drupal.org/documentation/git/interdiff
Comment #19
pbull CreditAttribution: pbull commentedTested #16 in OSX (10.9.5) under Firefox 35.0, Safari 7.1.2, Chrome 39.
Looks good in Bartik, Stark and Classy.
Comment #22
ruthsieg CreditAttribution: ruthsieg commentedTested #16 on windows 7 home premium sp 1 under Firefox 34.0.5, IE 11.0.9600, Chrome 39.0.2171.99.
Bartik, Stark, Classy and Seven look fine.
Comment #23
davidhernandezWell, it's green again. Did we have temporary bot problems?
Comment #24
pbull CreditAttribution: pbull commented@davidhernandez the first run failed in an unrelated test in ViewExecutableTest.
Comment #25
ruthsieg CreditAttribution: ruthsieg commentedComment #26
ruthsieg CreditAttribution: ruthsieg commentedComment #27
dernetzjaeger CreditAttribution: dernetzjaeger commentedComment #28
dernetzjaeger CreditAttribution: dernetzjaeger commentedchanged markup in tablesort-indicator.html.twig to
so we don't need template_preprocess_tablesort_indicator in theme.inc, also we can change the indicator- style themewise with only drop some lines of css.
All Drupal Core Themes should then add tablesort-indicator.css in their css components.
Comment #29
joelpittet@dernetzjaeger thank you for the patch and the idea. It was RTBC'd before so will this change give that much improvement? Glad to see the preprocess go away, so that's nice:)
Is there supposed to be CSS with this new patch? Can you somehow get the accessibility back via aria-descriptions or something?
Here's a bit of Twig/class name review:
Minor nit pick but don't put a space before the starting print curlies but do but it after because when attributes print it puts a space before each attribute printed.
And 'desc' would be slightly more preferable to 'dsc' as we usually don't use abbreviations with missing letters.
I'm fine with the previous RTBC patch in #16 as well except th img {} is a bit too generic of a selector for the CSS, if this issue goes back to that, I think that should be addressed if possible.
Comment #30
dernetzjaeger CreditAttribution: dernetzjaeger commented@joelpittet it was RTBC'd and then resetted to needs work, that was confusing.
Yes, i think it's an improvement, because if you would like to change the indicator style, you don't need to rewrite the preprocessing function.
No, there is no css in that patch, because I think that is more theme related. We should then open issues for each theme. Here is a related issue for Seven Clean up images used in the Seven theme.
ARIA: sure, you are right!
We could add aria-sort attribute to both spans.
Thanks for your notes and yeah "desc" is quit better ;)
Comment #31
davidhernandezIf the graphic is being removed in favor of a css based solution, there should be a core equivalent. The arrows are functional and not just for styling.
Comment #32
dernetzjaeger CreditAttribution: dernetzjaeger commented@davidhernandez
Yes, of course we need to add the arrows to a core css.
but the positioning can be done in theme css using .tablesort class.
Comment #33
dernetzjaeger CreditAttribution: dernetzjaeger commentedComment #34
LewisNymanGood work, love this patch! I did a few things here:
Stark before:
Stark after:
Seven before:
Seven after:
Comment #36
idebr CreditAttribution: idebr commentedThe text for screenreaders is causing the test to fail. I wonder if this could go in a followup or if it should be fixed in this issue?
Comment #37
davidhernandez+<span {{attributes.addClass( classes ) }}>
Spacing here needs to be fixed. Should be:
+<span{{ attributes.addClass(classes) }}>
Comment #38
davidhernandez@idebr, all patches have to pass testing or they won't get committed, so it will have to be fixed here.
Comment #39
idebr CreditAttribution: idebr commented@davidhernandez Yes, I'm aware :). I was just wondering if the additional text for screenreaders should be added in this issue or in a followup, since I didn't expect it to be included in this issue based on its current title and issue summary.
Comment #40
emma.mariaAdded the fix mentioned #37.
Comment #41
joelpittet@emma.maria thank you for fixing that.
@idebr Do you know why screenreader tests are failing?
@LewisNyman et al Should those new SVGs be added to seven theme folder as their specific to that theme?
Comment #43
idebr CreditAttribution: idebr commentedOld markup for a clicksortable table header:
New markup for a clicksortable table header:
The test fails on
Comment #44
LewisNymanI think they should go in the global icons folder, just so they are easily available for contrib
Comment #47
nlisgo CreditAttribution: nlisgo commentedComment #48
nlisgo CreditAttribution: nlisgo commentedComment #49
Manjit.Singhrerolling patch :)
@lewisnyman Can you please verify
Comment #50
davidhernandezWhat is the difference between #48 and #49? Please upload an interdiff. #49 was green, so I don't know why it needed to be rerolled.
Comment #51
Manjit.Singh@davidhernandez Actually i was talking to lewisnyman on IRC about this issue, After that i have reroll a patch and upload it but forget to reload browser. But in the mean time nlisgo also uploaded the same.
Comment #52
nlisgo CreditAttribution: nlisgo commentedHappy for my file to be hidden and the patch @Manjit.Singh provided to be put forward for review but an interdiff would be useful as there does appear to be a small difference in the patch files.
Comment #53
Manjit.Singhcreating interdiff first time, can you please check whether i have'nt done it wrong :)
Comment #54
nlisgo CreditAttribution: nlisgo commentedThere were some issues with the interdiff in #53. I use the 'Create interdiff using git' approach documented in https://www.drupal.org/documentation/git/interdiff
Here is the interdiff.
Comment #55
nlisgo CreditAttribution: nlisgo commentedThere are some issues with both patches in #48 and #49 but less with #48 so I will resupply a patch with an interdiff with #48.
Feedback for the patch in #49:
The svg paths have a fill colour of #787878 but I believe this should be the fill colour #004875 or #008ee6 depending on which folder the svg is in.
The name of the tablesort-indicator.css.css file is incorrect and should be tablesort-indicator.css
I incorrectly applied the patch to system.theme.css in #48 which is why I need to prepare it again.
Comment #56
nlisgo CreditAttribution: nlisgo commentedThe interdiff highlights the mistake in the patch prepared in #48 but this is really just a reroll of #40.
Please review.
Comment #57
Noe_ CreditAttribution: Noe_ at Devhouse Spindle commentedI love the fact that someone is taking the time to make the PNG's into SVG's. It saves a lot of space and it automatically scales for retina devices, so that is great.
However, it doesn't load the SVG's. If I apply the patch (which it does brilliantly) and I go to /admin/people I get nothing.
<a href="/admin/people?user=&role=All&permission=All&status=All&=Filter&order=created&sort=asc" title="sort by Member for"> Member for <img src="" width="13" height="13" alt="sort ascending" title="sort ascending" /> on Seven.
Here is a picture from the Chrome Devtools inspector:
Also, I don't see anything from the tablesort-indicator.html.twig when I search for it in the source code.
I really do hope this get's into Drupal8.
Comment #58
cilefen CreditAttribution: cilefen commented@Noe_ I see the same.
Comment #59
nlisgo CreditAttribution: nlisgo commentedNeeded to get rid of the template in the classy theme also. See the interdiff.
Comment #61
nlisgo CreditAttribution: nlisgo commentedAddressed failing tests.
Comment #62
Noe_ CreditAttribution: Noe_ at Devhouse Spindle commented#61 looks pretty good. It just works on the Seven theme right?
I changed the theme to Bartik and there it doesn't work.
Comment #63
LewisNymanThis patch should work in Bartik, Stark, and Classy. We should upload before/after screenshots.
@Noe_ It looks like you might need to clear your cache or reinstall, you are still loading the old templates.
The only other point here is that this patch does a lot more than it states in the title and issue summary. We are removing a template and replacing it with CSS. We need to make that clear in the issue summary and title, we also need sign off from a theme system maintainer, or we should move that change to a another issue where it can be discussed separately.
This also requires a blank line
This requires a blank line after the comment
Comment #64
LewisNymanManjit pointed out on IRC that this is not a @file comment so it doesn't need a blank line below.
Comment #65
Manjit.Singh@lewisnyman Please verify.
Comment #66
LewisNymanYep that looks good, we just need a theme system maintainer to verify that we can remove a template in this issue. Joel has been involved so assigning to him.
Comment #67
joelpittetI love the way this is looking, less need for preprocess to add an image is a big win IMO.
Template removal should be fine.
We should get some screenshots and Issue Summary Update resolved.
got an extra space in there before the {{, that can be removed.
Do these need periods? It wasn't before, the capitol is fine though I think(remember to change the click link test if you remove the period.
Comment #68
Manjit.Singh@joelpittet Please find and review the patch :)
Comment #69
joelpittetPlease remove the periods from these too.
Comment #70
Manjit.SinghComment #71
Manjit.SinghComment #73
nlisgo CreditAttribution: nlisgo commentedAddressing feedback in #69.
Comment #74
nlisgo CreditAttribution: nlisgo commentedComment #79
Manjit.Singh@nlisgo Patch is looks like good... but don't know why testing is failed everytime.
@lewis Can you have some time to look upon.
from myside all (UI) looks good :)
Comment #80
nlisgo CreditAttribution: nlisgo commented@Manjit.Singh sometimes testbot will fail but it is not due to the code.
See: https://www.drupal.org/automated-testing/troubleshooting
I was not able to reproduce the error locally so I determined that it was an issue with the testbot and simply queued the patch for a retest.
Comment #81
LewisNymanNice, I've updated the issue summary and here are the screenshots for Seven.
Comment #82
Manjit.SinghScreenshots looks fine for me !! thanks lewis
Comment #83
alexpottThis needs a change record
Comment #84
cilefen CreditAttribution: cilefen commentedI started a draft CR.
Comment #85
cilefen CreditAttribution: cilefen commentedA novice can finish the change record.
Comment #86
LewisNymanI would be nice to show the markup before and after, as well as how someone would override the icons after this patch.
Comment #87
cilefen CreditAttribution: cilefen commentedIn case you are wondering, this information is needed in the change record.
Comment #88
deepakaryan1988Removing sprint weekend tag!!
As suggested by @YesCT
Comment #89
karolus CreditAttribution: karolus as a volunteer commentedTried to apply the patch, changes to Seven theme YAML file.
Comment #90
wmortada CreditAttribution: wmortada as a volunteer commentedI've re-rolled this patch against the latest commit (251ca21).
There was a conflict in the file core/themes/seven/seven.libraries.yml - an additional CSS file was added by this patch and in the main branch. I kept both CSS files.
I've tested the changes and it appears to work as expected.
Comment #91
karolus CreditAttribution: karolus as a volunteer commentedJust tested the #90 patch--all appears to be in working order, tested under Classy, Bartik, and Stark themes. To be sure that things were OK, a few displays were checked across the site.
Comment #92
karolus CreditAttribution: karolus as a volunteer commentedManually tested the #90 patch across different displays in Classy, Stark, and Bartik themes. Also did a review of all lines in the patch, and they appear to be in the scope of the issue (but could stand to have more experienced eye review) of the patch code, and changes appear OK. Switching back to the 8.0.x branch to compare gave further indication the patch is working as advertised.
Comment #93
karolus CreditAttribution: karolus as a volunteer commentedWhile going back to make screenshots to document the patch changes, I noticed another issue--In the Bartik theme, the arrows appear to disappear, since they are the same color as the default table heads. This is evident in the highlighted file.
Comment #94
karolus CreditAttribution: karolus as a volunteer commentedComment #95
karolus CreditAttribution: karolus as a volunteer commentedAdded remaining tasks to issue summary. Am now going to work on change record.
Comment #96
karolus CreditAttribution: karolus as a volunteer commentedComment #97
karolus CreditAttribution: karolus as a volunteer commentedNew screen grabs, to be referenced in the issue summary.
Comment #98
karolus CreditAttribution: karolus as a volunteer commentedIssue summary updated.
Comment #99
karolus CreditAttribution: karolus as a volunteer commentedThere is a draft change record now--the first I've ever written. Link
Comment #100
clacina CreditAttribution: clacina at Drupal Association commentedI'm going to fix the problem with the Bartik theme that was mentioned in comment #93
Comment #101
amanda.drupal CreditAttribution: amanda.drupal at Drupal Association commentedI'm checking the beta evaluation
Comment #102
amanda.drupal CreditAttribution: amanda.drupal at Drupal Association commentedI'm not sure if this is unfrozen because it does change the PHP test and a yml file but both of those look like minor changes.
Comment #103
clacina CreditAttribution: clacina at Drupal Association commentedI have confirmed that it works in every theme except for Bartik.
This was part of a Drupal Association sprint. The sprint is over, so other people are free to take over the issue.
Comment #104
mgifford@clacina - what was your plan for Bartik? We can't change the default table heads for Bartik I assume.
I'm assuming we should just create a new twistie-down.svg & twistie-up.svg pair for Bartik that is a lighter shade. I think this is the grey that is closer to what is there #b6b6b6 so we could just create a:
core/themes/bartik/images/b6b6b6/twistie-down.svg
Comment #105
joelpittetHere's a re-roll because a few things moved around. For bartik, I suggest providing some svg icons with better colors. Because it is there but the color just matches the background. @emma.marie do you have a bartik color palette to work with for those colors?
In the re-roll I moved
core/modules/system/css/components/tablesort.theme.css
and what added to
core/modules/system/css/system.module.css
into
core/modules/system/css/components/tablesort.module.css
Hope I got the name correct (barely understand module/theme structure)
Comment #106
joelpittetOh also added this for RTL.
Comment #107
mgiffordComment #108
avitslv CreditAttribution: avitslv commentedFor bartik added
White icons on gray background. See screnshot how it looks and should be reviewed by theme maintainer.
Comment #109
jiv_e CreditAttribution: jiv_e as a volunteer commentedWhat's happening in system.libraries.yml? There's 'every_page: true' added on every css component. Is this intended and what does it do?
Comment #110
davidhernandezevery_page
was removed from core so that shouldn't be there. @avitslv make sure your development copy of Drupal 8 is up to date with the latest head. You should see those every_page entries go away.Comment #111
avitslv CreditAttribution: avitslv commentedResubmitted patch. Fixed the mixup for system.libraries.yml, thanks @jiv_e @davidhernandez
This time added Desc arrow screenshot for bartik:
Comment #112
avitslv CreditAttribution: avitslv commentedComment #113
emma.mariaI checked out the arrow-asc and arrow-desc in Bartik. They look great! Such a visual improvement for Bartik having the arrows white and the arrows work as expected in all tables. Thanks everyone!
Screenshots for good measure.
Comment #114
joelpittetThere seems to be a couple change records so removing that tag.
Had a quick glance at Seven theme and it's still good too.
Comment #117
LewisNymanBack to RTBC
Comment #119
jaxxed CreditAttribution: jaxxed at Wunder commentedpatch #111 needs a reroll
Comment #120
davidhernandezComment #121
madhavvyas CreditAttribution: madhavvyas as a volunteer commentedPatch rerolled #111
Comment #122
madhavvyas CreditAttribution: madhavvyas as a volunteer commentedComment #123
mgiffordShouldn't this patch also remove core/misc/arrow-asc.png core/misc/arrow-desc.png and any references to them?
Comment #124
madhavvyas CreditAttribution: madhavvyas as a volunteer commentedPatch updated as per #123
Comment #125
madhavvyas CreditAttribution: madhavvyas as a volunteer commentedForget to remove reference for arrow-asc and arrow-desc files
Comment #126
LewisNymanIt looks like this patch is missing the changes to the twig file.
These lines could also be removed, because they are changing the src for the img tag instead of in CSS.
Comment #127
nlisgo CreditAttribution: nlisgo commentedThe sizes of the patch files for #121, #124 and #125 are significantly smaller than the patch in #111 which was RTBC. I just quickly attempted a re-roll and the size of my patch file is around 12K. So something has gone wrong.
Here is a re-roll of #111 for review.
Comment #128
emma.maria@nlisgo you beat me to it I agree with a reroll of #111. The patch in #127 contains everything that was in #111.
Setting to RTBC when the patch goes green.
Comment #129
LewisNymanI manually tested this patch and it looks the same as the screenshots above.
Comment #133
davidhernandezNew CI passes. Old testbot has AWS errors?
Comment #135
davidhernandezSetting back to RTBC per #129. The fails were testbottiness.
Comment #136
alexpottMarkup and templates are yet frozen. And bartik looks much better - nice work everyone. Committed 8bcdbf2 and pushed to 8.0.x. Thanks!