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

Feature/delete all dowload sources #1399

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

KelvinDiasMoreira
Copy link
Contributor

@KelvinDiasMoreira KelvinDiasMoreira commented Jan 14, 2025

When submitting this pull request, I confirm the following (please check the boxes):

  • I have read and understood the Contributor Guidelines.
  • I have checked that there are no duplicate pull requests related to this request.
  • I have considered, and confirm that this submission is valuable to others.
  • I accept that this submission may not be used and the pull request may be closed at the discretion of the maintainers.

Fill in the PR content:

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR adds functionality to delete all download sources at once across multiple language files and components. Here's a focused summary of the key changes:

  • Multiple language files have incorrect translations for 'removed_download_sources', using "fonts removed" instead of "download sources removed" (affecting bg, da, de, hu, id, kk, ko, ru, tr, zh)
  • Added new ConfirmationModal prop buttonsIsDisabled to support disabling both confirm/cancel buttons during deletion
  • Implemented deleteAllDowloadSources function in worker with a typo in the name and missing error handling
  • Added CSS styling for a new "Remove All Sources" button positioned with flex-end justification
  • Added bulk deletion UI flow in settings-download-sources.tsx with confirmation modal and state management

The implementation needs several fixes for translation accuracy and error handling robustness.

💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!

32 file(s) reviewed, 51 comment(s)
Edit PR Review Bot Settings | Greptile

src/locales/bg/translation.json Show resolved Hide resolved
src/locales/be/translation.json Show resolved Hide resolved
src/locales/be/translation.json Outdated Show resolved Hide resolved
src/locales/be/translation.json Outdated Show resolved Hide resolved
src/locales/be/translation.json Outdated Show resolved Hide resolved
@thegrannychaseroperation
Copy link
Contributor

@KelvinDiasMoreira code looks good. Could you attach a screenshot on how the interface looks like?

@KelvinDiasMoreira
Copy link
Contributor Author

@KelvinDiasMoreira code looks good. Could you attach a screenshot on how the interface looks like?

image

image

@KelvinDiasMoreira
Copy link
Contributor Author

@Hachi-R
Copy link
Collaborator

Hachi-R commented Feb 3, 2025

@zamitto, @thegrannychaseroperation

@KelvinDiasMoreira the last time i tested your pr, the worker had an issue that was preventing me from importing fonts, also you didnt address my request to move the button up

could you fix those?

@Hachi-R Hachi-R self-assigned this Feb 3, 2025
@KelvinDiasMoreira
Copy link
Contributor Author

@zamitto, @thegrannychaseroperation

@KelvinDiasMoreira the last time i tested your pr, the worker had an issue that was preventing me from importing fonts, also you didnt address my request to move the button up

could you fix those?

No message came to me about this request, but today i will do this change. About the issue i think that got fixed, but i'll test tonight.

Copy link

sonarqubecloud bot commented Feb 4, 2025

@KelvinDiasMoreira
Copy link
Contributor Author

@Hachi-R, i test here the import fonts after delete the fonts and works fine, can you test again in this last commit 026abd7 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants