Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moved ajax-loader.gif to achieve better caching #9167

Merged
merged 5 commits into from
May 13, 2024

Conversation

Spedi
Copy link
Contributor

@Spedi Spedi commented Apr 28, 2024

Closes #9104

Moved ajax-loader.gif from the css folder to the images one, to achieve better caching, and updated all deprecated references to the file.

Stakeholders

@RayBB

@Spedi
Copy link
Contributor Author

Spedi commented Apr 28, 2024

Hi @RayBB, I'm not sure why there's 3 commits to this PR, since I created a new branch just for the caching issue.
Please see the changes and let me know if I did something wrong (which is highly probable). Also I updated the references on 2 files but I can't see them in the commits here on Github, not sure why though.

@Spedi
Copy link
Contributor Author

Spedi commented Apr 28, 2024

Got why I didn't see the update on references, didn't do git add .... noob error I suppose.
Nevertheless I can't understand why the other commits (Those about Issue #9135) are on this PR, maybe you can help me on elucidating the issue there.

@RayBB
Copy link
Collaborator

RayBB commented Apr 28, 2024

@Spedi you could just undo the changes related to old-style-lists and push that in a new commit.

@Spedi
Copy link
Contributor Author

Spedi commented Apr 29, 2024

@RayBB Done that, it was an easy fix indeed.
Nonetheless, do you know why this stuff happened? I'm a newbie to the Git world so I may have messed up something in the process, it'll be very helpful to have some kind of feedback.
Thank you so much for your help and patience, really appreciate it, I'm learning a lot by working on this project! :)

Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested locally and is working great!
Just need staff to merge then I'll update my other PR :)

@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label Apr 30, 2024
@mekarpeles
Copy link
Member

The changes make sense, though I'm confused why we replaced the transparent version of the gif with one having a white background -- that may have some visual consequences.

Cc: @RayBB

@mekarpeles mekarpeles added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label May 6, 2024
@RayBB
Copy link
Collaborator

RayBB commented May 6, 2024

@mekarpeles idk why github show sit as not transparent but this is what it looks like in prod already.
image

The "transparent version" isn't used anywhere at all that's why we're removing it.
Personally, I think it would be quite nice to have a fancy svg loading indicator but I'll leave that to the design team to decide on :)

@mekarpeles mekarpeles merged commit 61ffbe4 into internetarchive:master May 13, 2024
4 checks passed
@Spedi Spedi deleted the better-caching branch May 17, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move ajax-loader.gif for better caching
3 participants