- ๐ฎ๐ณIndia Gauravvv Delhi, India
Gauravvvv โ made their first commit to this issueโs fork.
- Status changed to Needs review
25 days ago 4:07am 22 May 2024 - ๐ญ๐บHungary Balu Ertl Budapest ๐ช๐บ
Balu Ertl โ made their first commit to this issueโs fork.
- Status changed to Needs work
23 days ago 9:52pm 23 May 2024 - ๐ช๐จEcuador jwilson3
I setup a codepen to review the before (GIF) and after (SVG) from the MR.
https://codepen.io/jameswilson/pen/wvbWNbr
Things are really close and this is exciting!
I found a few minor but important things to consider:
- The SVG duration is currently 1.0 seconds, but the GIF duration is 0.8 seconds (verified by uploading the current
<a href="https://git.drupalcode.org/project/drupal/-/raw/d40360cb42b154235c5ec3f2edf7696f211f5726/core/misc/loading-small.gif">loading-small.gif</a>
to ezgif.com (here). One second appears slower and more lethargic, which is not what you want to portray when displaying a loader. There needs to be a slight sense of urgency that 0.8 seconds provides, versus 1 second. - The GIF size is 24 x 24 pixels, the SVG is 25 x 25. I suggest we maintain the same dimensions by default if possible (people can always resize it with CSS later, and since its a vector it will still look good!
- Needs testing on simplytest.me with a before/after screenshot to ensure the rounded corners applied via CSS still look good.
- Issue needs summary update, and we should hide all the old patches, attachments, and approaches.
- The SVG duration is currently 1.0 seconds, but the GIF duration is 0.8 seconds (verified by uploading the current
- ๐ช๐จEcuador jwilson3
I've pushed fixes for the two items, and created a new codepen for comparison:
0.8s at 24px
https://codepen.io/jameswilson/pen/oNRLVxx
Compare against previous pen with 1s at 25px
- ๐ช๐จEcuador jwilson3
Hide file attachments from previous approaches in the issue summary.
- ๐ช๐จEcuador jwilson3
Remaining tasks:
- Needs replacement for loading.gif => loading.svg (with roughly the same SVG used for loading-small.svg, only with different dimensions.
- Needs testing on simplytest.me with a before/after screenshots of loading.svg and loading-small.svg, to ensure no visual regressions.
- ๐ช๐จEcuador jwilson3
I found an instance of loading-small.gif in the starterkit_theme that can be removed:
./core/themes/starterkit_theme/images/icons/loading-small.gif
- Status changed to Needs review
23 days ago 11:01pm 23 May 2024 - ๐ช๐จEcuador jwilson3
Since we're deleting the GIFs, we may want to consider adding a
.htaccess
rules to redirect the requests for the deleted GIF images to their respective SVG image counterparts. Thoughts? - ๐ช๐จEcuador jwilson3
I've created a comparison codepen for loading.gif => loading.svg
- Status changed to Needs work
16 days ago 1:54pm 31 May 2024 - ๐บ๐ธUnited States smustgrave
May need a rebase. Reran the unit tests multiple time but fails each.
- Status changed to Needs review
14 days ago 6:59pm 1 June 2024 - ๐ช๐จEcuador jwilson3
There are all kinds of seemingly unrelated test failures after the rebase. It seems the 11.x branch may be a bit unstable at the moment.
- Status changed to Needs work
12 days ago 2:42pm 4 June 2024 - ๐บ๐ธUnited States smustgrave
So brought this in #frontend and consensus seems to be these images could be used by third party modules so we can't delete. And there is no mechanism for deprecating images so can only remove in a major (D12)
So will need to add those images back and a CR in this ticket about the images being removed in 12
Opened ๐ [12.x] Remove images that have been replaced in core Active for D12 to remove those images later.
- ๐ช๐จEcuador jwilson3
Forgive my ignorance, but isn't D11 the next major?
- ๐บ๐ธUnited States smustgrave
There's already a beta out for 11 so believe we have missed the boat. Why most deprecations are now going to D12.
Don't need to delete now just need to add the deleted images back, still using the new svgs, and add a CR. Gives contrib modules time to update if they are using.
Then when 12 opens we have that one ticket as a reminder to go and delete the images.
- Status changed to Needs review
10 days ago 4:12am 6 June 2024 - ๐ช๐จEcuador jwilson3
I've restored core/misc/loading.gif and core/misc/loading-small.gif.
I did not restore the loading GIF in the starterkit theme, because by its very nature, it is not intended to be referenced, but instead forked/copied and manipulated. There's no need to ship starterkit with a file that will never be used.
And there is no mechanism for deprecating images so can only remove in a major (D12)
I get what you mean, and understand that the files can only be removed in a major upgrade, but I should clarify that the MR does include a mechanism for deprecating the old images which I think will work in the current D11 beta. Specifically:
- Make an addition to .htaccess to redirect requests to the old files to the new files.
- Ensure the new files are the exact same size and dimensions of the originals.
- Write a CR to explain that the files are deprecated in D11.0 (assuming this gets in soon) and that themes and modules should check for references to these files and update them accordingly for D12 and optionally D11.