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

Removed deprecated .png file #9246

Merged
merged 4 commits into from
May 14, 2024
Merged

Conversation

Spedi
Copy link
Contributor

@Spedi Spedi commented May 9, 2024

Closes #9237

Removed usages of icon_cancel.png, since it doesn't exist and it was still referenced in some part of the codebase.

Stakeholders

@RayBB

@rebecca-shoptaw
Copy link
Collaborator

rebecca-shoptaw commented May 9, 2024

Hi @Spedi ! Quick heads-up re: the pre-commit Generate POT check that this PR failed -- this is just because we added a new automatic action to update the translation files.

As soon as you update your master branch and rebase this one, the translations file will now be up to date and the test will pass, and you won't encounter this issue again with any branches off the updated master. 🙂

Steps to follow if needed:

git switch master
git pull upstream master
git push origin master
git switch your-branch
git rebase master

@Spedi
Copy link
Contributor Author

Spedi commented May 9, 2024

Thank you so much @rebecca-shoptaw!
Done those steps in Git, do I need to do anything else to make the PR work correctly?

@rebecca-shoptaw
Copy link
Collaborator

rebecca-shoptaw commented May 9, 2024

@Spedi Yes one last step! You can push the new changes to the PR as you normally would with a git push origin HEAD. If you get an error message, you can retry with git push -f origin HEAD, which will force an update.

And you shouldn't encounter something like this again, but this whole process is great to follow anytime you're returning to a PR you haven't worked on for a while or making a new branch -- keeps everything nice and up to date.

Comment on lines 7 to 8
background-repeat: no-repeat;
background-position: 100% 0;
Copy link
Collaborator

@RayBB RayBB May 10, 2024

Choose a reason for hiding this comment

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

Suggested change
background-repeat: no-repeat;
background-position: 100% 0;

Lets also remove these two since they don't do anything.
@Spedi

@rebecca-shoptaw
Copy link
Collaborator

rebecca-shoptaw commented May 10, 2024

@Spedi Quick update! It looks like the remote version your master branch is still out of sync with the OL master -- this seems to be the result of a manual merge conflict handling. To get everything back up to date, I'd recommend running:

# switches to master branch
git switch master

# undoes the last few commits on it, including the merge conflict
git reset --hard HEAD~3

# pulls in the OL master
git pull upstream master

# force pushes your changes to your remote branch
git push -f origin master

Once this is done, you should be able to switch to your branch and get it up to date as described:

# switches to last viewed branch
git switch - 

git rebase master
git push origin HEAD

Or git push -f origin HEAD if that fails!

P.S. Because your PR does involve removing translated text from the site (the alt and title for the icon), the pre-commit hook should successfully update the translation file for you when the PR is submitted.

@Spedi Spedi force-pushed the deprecated-file branch from b887d7e to 971bdcf Compare May 10, 2024 15:15
@Spedi
Copy link
Contributor Author

Spedi commented May 10, 2024

@rebecca-shoptaw done the steps, does everything look right now?
@RayBB implemented the suggested changes too.

@rebecca-shoptaw
Copy link
Collaborator

@Spedi thanks for running those commands! Weirdly it looks like some of them must have failed, as your remote master branch is still showing as out of date:
Screenshot 2024-05-10 at 3 37 57 PM

Happy to go back-and-forth on Slack or here to get this figured out! A question to get us started:

When you're on the local master branch (in VSCode or whatever IDE you use) and you navigate to the messages.pot file (/openlibrary/i18n/messages.pot), do you see the following creation date?

"POT-Creation-Date: 2024-05-01 18:58-0400\n"

If so, your local master is up to date, and it will just be a matter of getting your remote master and local/remote branch for the PR to match.

If not, I can help you get your local master back up to date and work from there. 🙂

@Spedi
Copy link
Contributor Author

Spedi commented May 11, 2024

@rebecca-shoptaw I see "POT-Creation-Date: 2024-04-17 18:20+0000\n", so I suppose that the local master is not up to date, what are the steps I need to do in order to resolve this issue?

@rebecca-shoptaw
Copy link
Collaborator

@Spedi All good! The following should fix it:

# on master branch
git reset --hard HEAD~10
git pull upstream master

You can check the date on the messages.pot file once this is done and it should be updated!

@Spedi
Copy link
Contributor Author

Spedi commented May 12, 2024

@rebecca-shoptaw I done those steps on git, but it doesn't seem like it's working, the messages.pot file still shows the old date. What do you think this happen?
Thank you for your patience ;)

@scottbarnes
Copy link
Collaborator

pre-commit.ci run

@scottbarnes
Copy link
Collaborator

scottbarnes commented May 12, 2024

@Spedi, I can't tell if you can fix this by simply running pre-commit run generate-pot --all-files locally and adding the updated openlibrary/i18n/messages.pot to your PR, or whether this won't work until #9258 is merged and you rebase off master (or you make similar changes).

We may need to change the pre-commit rule a bit if #9258 doesn't stop this from happening.

@rebecca-shoptaw
Copy link
Collaborator

rebecca-shoptaw commented May 12, 2024

@Spedi Very weird that the upstream pull isn't going through! I'll start a back and forth with you in Slack so you can send screenshots/etc. and we can figure it out. 🙂

And I can confirm that the error messages @scottbarnes noticed, while annoying and on their way to being fixed, shouldn't be what's holding this up! Once the branch is correctly up to date you should be all good.

@cdrini cdrini force-pushed the deprecated-file branch from 971bdcf to 9d4b2ca Compare May 13, 2024 17:57
@cdrini
Copy link
Collaborator

cdrini commented May 13, 2024

It looks like something got tangled with your master branch @Spedi ; this can happen when one accidentally commits something to the local master branch. Run this to clean it up to avoid issues in future PRs:

git checkout master
git fetch upstream master
git reset --hard FETCH_HEAD

This resets it to be identical to the internetarchive/openlibrary master branch.

I've fixed up this branch now since it's a small branch which we should be able to quickly merge 👍

@cdrini cdrini self-assigned this May 13, 2024
@cdrini cdrini assigned RayBB and unassigned cdrini May 13, 2024
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.

Just confirmed on testing and it looks good to me!
image

@RayBB RayBB removed their assignment May 13, 2024
@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label May 13, 2024
@cdrini cdrini merged commit ddaab7e into internetarchive:master May 14, 2024
5 checks passed
@Spedi Spedi deleted the deprecated-file branch May 17, 2024 22:04
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove usages of missing icon_cancel.png
5 participants