Skip to content

blocklist the bounces option #2409

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Bowrna
Copy link
Contributor

@Bowrna Bowrna commented Apr 15, 2025

As my old PR got closed, raising this one again.
closes: #2105

Edit: Life's merge conflict delayed this PR, but the rebase is complete now!

@Bowrna
Copy link
Contributor Author

Bowrna commented Apr 15, 2025

Some places where I could have done better.

  1. Currently, I kept the two buttons "blocklist all" and "clear all" visible always and selected actions to appear on select.
Screenshot 2025-04-15 at 10 45 44 PM Screenshot 2025-04-15 at 10 45 53 PM 2. For the selected action drop-down menu, I am using the arrow-down icon and arrow-up icon as I couldn't add the icons for drop-down. I tried adding, but I had to generate the woff2 file, which I tried, but I couldn't as the number of icons was too big in the fontello.css to regenerate ( I am not sure if there is a better way to tackle this problem) Screenshot 2025-04-15 at 10 46 01 PM 3. Unlike clear or clear all which removes an entry from bounces table and vanishes from bounces, blocklist or blocklist all doesn't do anything that indicates in UI of the bounces page that these bounces subscribers are blocklisted. If there is a suggestion on how to handle this in better way through frontend let me know.

@MaximilianKohler
Copy link
Contributor

MaximilianKohler commented Apr 16, 2025

When checking the select all box next to email, does it select all on the current page, or all on every page? I think the optimal solution is to select all on the current page and then have some text and a button: 25 selected; click here to select all 250

EDIT: nvm the last part, since there is already a "blocklist all" button. But that button could be removed for the above sequence instead.

And maybe at some point the list of bounces could be sortable by "hard", "soft", so we could select only the hard bounces to block. EDIT: nvm, it's sortable currently, it just doesn't work on v2.5.1 but maybe this is fixed already in newer versions.

@Bowrna
Copy link
Contributor Author

Bowrna commented Apr 16, 2025

@MaximilianKohler when checking the checkbox before "Email" It does select all on the current page alone.
I retained the working of the existing flow where the "Clear all" button is always visible and "Clear" button appears on checking the checkbox for that page. Similary I added "Blocklist all" which applies blocklist for all the bounces and for selected checkbox, I made the Selected actions button visible with drop down option to blocklist or clear.

Regarding sortable by "hard", "soft", I will add that as seperate PR once this one gets merged.

@MaximilianKohler
Copy link
Contributor

Probably not a big deal, but this is how it currently works when selecting subscribers in lists:
select count

To keep things consistent, you may want to mirror that behavior.

Then you could remove the "blocklist all" button, and even the "clear all" button.

@Bowrna
Copy link
Contributor Author

Bowrna commented Apr 17, 2025

Yes it makes more sense to do it that way. Let me update in this PR @MaximilianKohler

@Bowrna
Copy link
Contributor Author

Bowrna commented Apr 22, 2025

@MaximilianKohler I have implemented the changes you have suggested.

Edit: @knadh Could you review the changes and share your feedback?

@Bowrna
Copy link
Contributor Author

Bowrna commented Apr 23, 2025

@knadh Somewhat out of the box discussion wrt this particular PR, but I initially stumbled upon using icons for this PR to support drop down, and found that listmonk uses Fontello. While the menu-up and menu-down icons are not bundled, I tried to see if they can be added. I tweaked it by checking with frontend/fontello/config.json and faced some issues where I couldn't upload many of the icons due to the upper limit for upload at fontello end. The current config.json supports multiple icons, and the file is around 3.5MB, and we don't use many of the icons present in it. Considering all , could we consider support for SVG based icon library like heroicons that would allow us to add only necessary icons in the bundle?

@knadh
Copy link
Owner

knadh commented Apr 23, 2025

Hi @Bowrna,
Did you follow these instructions? https://github.com/knadh/listmonk/tree/master/frontend#icon-pack

There are no issues with dragging-dropping config.json onto Fontello's frontend. I was just able to do it.
image

Fontello allows selecting only the icons that are required in one place without bringing them into the frontend JS code. We can explore the tradeoffs and consider SVG icons for a future release.

@Bowrna
Copy link
Contributor Author

Bowrna commented Apr 24, 2025

Hi @knadh, Thanks for the update. I missed reading this part of the documentation. When I tried to upload the config.json, I could see only a few icons from the config file getting loaded on the screen. I assumed it was the limitation at fontello as only 43 icons present in the config.json got loaded in the custom icons. Just after checking this docs, I could understand that only 43 icons whose JSON has key "selected" as true value are shown as selected.

@Bowrna Bowrna force-pushed the blocklist_bounces_recent_fix branch from 7b6aaca to a825ac7 Compare April 25, 2025 06:21
@Bowrna Bowrna force-pushed the blocklist_bounces_recent_fix branch from a825ac7 to b2a3487 Compare April 25, 2025 06:26
@Bowrna
Copy link
Contributor Author

Bowrna commented Apr 30, 2025

Could this PR be reviewed at your convenience? @knadh

@knadh
Copy link
Owner

knadh commented Apr 30, 2025

Just wound up marathon work on the v5 release. I'll review this PR maybe a month or two from now, closer to the next release. Thanks @Bowrna.

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.

[feature request] ability to bulk blocklist users from the Bounces page
3 participants