Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Proposed resolution
There are quite a few images in the Seven theme that are not being used.
Remaining tasks
For each image, check it is not being used; if they are, replace them with the correct Libricon in core/misc/icons
User interface changes
Some nice and shaper icons ;-)
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#28 | interdiff-2407955-24-28.txt | 445 bytes | darketaine |
#28 | 2407955-28.patch | 5.12 KB | darketaine |
#24 | 2407955-24.patch | 5.19 KB | ironkiat |
#20 | issue-2407955-20.patch | 5.19 KB | ChuChuNaKu |
#13 | issue-2407955-13.patch | 7.95 KB | dernetzjaeger |
Comments
Comment #1
dernetzjaeger CreditAttribution: dernetzjaeger commentedComment #2
dernetzjaeger CreditAttribution: dernetzjaeger commentedchecked all files
found 2 references in tablesort-indicator.html.twig and forms.css.
changed markup in tablesort-indicator.html.twig to
added css/components/tablesort-indicator.css
Comment #3
dernetzjaeger CreditAttribution: dernetzjaeger commentedfound missing @file in forms.css and forgot "." on @file comment in tablesort-indicator.css
Comment #4
dernetzjaeger CreditAttribution: dernetzjaeger commentedComment #5
LewisNyman@dernetzjaeger Thanks for the work here. I think you're miss the extra table sort indicator file. The best way to include it in the patch is to make a commit adding a file and then do
git diff HEAD~1
to create the patch.Also, I do agree with the CSS only changes in principle, but I think we should make that change globally across core in another issue (I can't find it right now)
Comment #6
dernetzjaeger CreditAttribution: dernetzjaeger commented@LewisNyman Yes, just saw tha the file is missing. I had some trouble with git, sorry for that.
I think it's that issue Replace arrow-asc and arrow-desc images with Libricons
Comment #7
dernetzjaeger CreditAttribution: dernetzjaeger commentedI just investigated that a bit.
We need to make changes to modules/system/templates/tablesort-indicator.html.twig as shown in #2.
Then we could remove
from theme.inc
In the next step all Themes can implement classes .tablesort and .tablesort--asc/dsc to customize the table- sorting item as prefered.
Comment #8
dernetzjaeger CreditAttribution: dernetzjaeger commentedComment #9
dernetzjaeger CreditAttribution: dernetzjaeger commentedComment #10
stefan.r CreditAttribution: stefan.r commentedThis also removes some unused files:
Comment #11
LewisNymanLooking good! We have a bit of a regression here with the size and the color of the table sort arrows. I have taken before and after screenshots:
Before
After
Comment #12
dernetzjaeger CreditAttribution: dernetzjaeger commentedComment #13
dernetzjaeger CreditAttribution: dernetzjaeger commented@stefan.r
Thanks :)
@LewisNyman
I didn't add the template file to the patch and if that got applied, we won't need an extra template file in Seven.
After
Comment #14
LewisNyman@dernetzjaeger Ah ok, shall we postpone this issue on #2405057: Replace arrow-asc and arrow-desc images with Libricons and implement using CSS?
Comment #15
mgiffordComment #16
emma.mariaUn-postponing as the issue mentioned in #14 was committed and the Seven is not frozen for changes, so clean up tasks can take place.
We just need to check that the most recent patch in #13 has everything we need in it.
Comment #17
falufalump CreditAttribution: falufalump commentedThe most recent patch (#13) does not currently apply cleanly. It may need a re-roll.
Comment #18
emma.maria@mccartj5 yes, if a patch cannot be applied cleanly it definitely needs a re-roll. Feel free next time to set the issue to "Needs Work" and also tag it with "Needs reroll".
Comment #19
ChuChuNaKu CreditAttribution: ChuChuNaKu as a volunteer commentedWe're taking a look at this as part of the sprint weekend.
Comment #20
ChuChuNaKu CreditAttribution: ChuChuNaKu as a volunteer commentedPatch #13 generated the following errors:
We (leslieg and chuchunaku) made the following changes and re-rolled the patch:
deleted: core/themes/seven/images/add.png
deleted: core/themes/seven/images/arrow-asc-active.png
deleted: core/themes/seven/images/arrow-asc.png
deleted: core/themes/seven/images/arrow-desc-active.png
deleted: core/themes/seven/images/arrow-desc.png
deleted: core/themes/seven/images/arrow-next.png
deleted: core/themes/seven/images/arrow-prev.png
deleted: core/themes/seven/images/required.svg
deleted: core/themes/seven/images/task-check.png
deleted: core/themes/seven/images/task-item-rtl.png
deleted: core/themes/seven/images/task-item.png
modified: core/themes/seven/css/components/form.css
Comment #21
Bojhan CreditAttribution: Bojhan commentedLooks good to me!
Comment #22
ironkiat CreditAttribution: ironkiat at Pixel Onion Pte Ltd commentedApplied the patch, successfully removed all the images in seven theme.
All looks good to go!
Comment #23
star-szrThanks, this looks pretty reasonable (all the images removed do look to be unused and all of the remaining images in Seven are used) but this one line looks a bit off - svgg instead of svg I think :)
Comment #24
ironkiat CreditAttribution: ironkiat at Pixel Onion Pte Ltd commentedUpdated patch file to remove that extra g in the css.
Comment #25
Bojhan CreditAttribution: Bojhan commentedMy bad, back to RTBC!
Comment #26
star-szrThanks! Will take a look soonish :)
Comment #27
star-szrSo I looked at this closer and Classy's form.css has this CSS:
That makes the background-image line redundant. We can rely on Classy here and remove the line.
Comment #28
darketaine CreditAttribution: darketaine as a volunteer commentedAccording to #27 here goes the patch
(maybe all the form-required:after is redundant?)
Comment #29
darketaine CreditAttribution: darketaine as a volunteer commentedComment #31
wellme CreditAttribution: wellme at Zyxware Technologies commentedIt is working. Can be made as RTBC.
Comment #32
mgifford@wellme that's great. Can you describe the tests you've done and change the status? Thanks!
Comment #33
wellme CreditAttribution: wellme at Zyxware Technologies commentedComment #34
wellme CreditAttribution: wellme at Zyxware Technologies commentedComment #35
mgiffordGreat. Thanks @wellme
Comment #36
star-szrThis is just cleanup so committing to 8.2.x only. Thanks all :)
Comment #38
Wim Leersd.o, you're weird. #36 is marked as "fixed", but here it says "needs review". WTF.