Skip to content

Allows eventcatcher to be disabled #9096

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

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

Conversation

saqimtiaz
Copy link
Member

This PR extends $eventcatcher with an optional disabled attribute that can be used to toggle the event listeners and thus the functionality of the $eventcatcher widget.

The salient code changes involve refactoring to introduce a new toggleEventListeners method, and updating the refresh logic to handle disabling and enabling the widget by removing and adding the event listeners respectively.

To do:

  • update documentation

Copy link

netlify bot commented Jun 15, 2025

Deploy Preview for tiddlywiki-previews ready!

Name Link
🔨 Latest commit 5502f62
🔍 Latest deploy log https://app.netlify.com/projects/tiddlywiki-previews/deploys/684f1fcf9343fe0008023b58
😎 Deploy Preview https://deploy-preview-9096--tiddlywiki-previews.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link

Confirmed: saqimtiaz has already signed the Contributor License Agreement (see contributing.md)

Copy link

📊 Build Size Comparison: empty.html

Branch Size
Base (master) 2528.0 KB
PR 2529.1 KB

Diff: ⬆️ Increase: +1.1 KB

@pmario
Copy link
Member

pmario commented Jun 15, 2025

I think the implementation is OK, but I am not happy with the toggleListeners name. It seems to be a "switchListeners" function, which takes a parameter, that defines the actual state of the listeners.

@saqimtiaz
Copy link
Member Author

@pmario the method in question removes or adds the event listeners depending on its argument. I am open to alternative names, however switchListeners does not seem appropriate as it implies swapping between one listener and another.

@pmario
Copy link
Member

pmario commented Jun 16, 2025

hmmm. The thesaurus also suggested "adjust" -- May be adjustListeners

@saqimtiaz
Copy link
Member Author

@pmario I do not find either of the proposed method names preferable to toggleListeners. Adjust would imply that the listeners are somehow modified, not that they are added or removed.

Toggle seems an apt description of what the method does, it adds and removes listeners. configureListeners could be an acceptable alternative, though toggleListeners feels more intuitive and self-explanatory to me.

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

Successfully merging this pull request may close these issues.

2 participants