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

[BUG]: Close Settings Modal on Clicking Outside of It #207

Closed
cdrani opened this issue May 16, 2024 · 24 comments · Fixed by #231
Closed

[BUG]: Close Settings Modal on Clicking Outside of It #207

cdrani opened this issue May 16, 2024 · 24 comments · Fixed by #231
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@cdrani
Copy link
Owner

cdrani commented May 16, 2024

When I click outside of the open Settings modal, I expect the modal to close.

image

Notable files:
https://github.com/cdrani/chorus/blob/develop/src/models/now-playing-icons.js
https://github.com/cdrani/chorus/blob/develop/src/models/chorus.js

I am looking specifically for implementation of a click outside event detection. Ideally it's written that this function is a util that can be added to any element (for future use).

Feel feel to pose comments/questions down below.

@cdrani cdrani added help wanted Extra attention is needed good first issue Good for newcomers labels May 16, 2024
@sstephanyy
Copy link
Contributor

sstephanyy commented May 18, 2024

@cdrani: Hello again. I forked the repo and tried to load the manifest file on both Google and Firefox, but I can't get rid of missing manifest.json error. Can you help me with this, please?

I'm trying to load the Firefox extension in this url: about:debugging#/runtime/this-firefox
Firefox version: 126.0

@cdrani
Copy link
Owner Author

cdrani commented May 20, 2024

@sstephanyy: Sure. By manifest file do you mean the manifest.json in the dist folder?

Common step is to run pnpm watch. This creates a dist folder.

For chrome, in chrome://extensions, select the dist folder in the File Selector when you click Load unpacked.

For firefox, there's a prior step where you have to update the manifest.json file.

Replace this:

"background": {
	"type": "module",
    "service_worker": "background.js"
}

with this

"background": {
	"type": "module",
    "scripts": [
    	"background.js"
    ]
},
"browser_specific_settings": {
	"gecko": {
    	"id": "chorus@cdrani.dev",
        "strict_min_version": "112.0"
	}
}

then in about:debugging, select the manifest.json file inside the dist folder.

Let me know if these resolves your current issue. I can create a quick walkthrough, but it will be just a visual of the steps outline in contributions local development. Mind posting screenshots of the steps taken so I can clue in on where maybe the newly written documentation differs from actual setup? This presents a good opportunity to update the documentation to ensure setup is always successful.

@sstephanyy
Copy link
Contributor

sstephanyy commented May 20, 2024

Hey, thanks for the quick walkthrough, but I'm still facing problems here...
"For chrome, in chrome://extensions, select the dist folder in the File Selector when you click Load unpacked."
I did what you told above, but the extension is broking on my Chrome (in Firefox too) everytime I switch the "on" and "off" button.
image

And this panel, that is in your project documentation, doesn’t appear for me.
image

Ow, I tried in the Firefox Add-Ons store, but it is happening the same problem.

@cdrani
Copy link
Owner Author

cdrani commented May 20, 2024

Make sure the extension has the required permissions.
image

Same for chrome if you click on details and scroll down to Site access.

Let me see screenshots of the results of these:

  1. Open console and screenshot any errors in there.
  2. See if there are any errors on the extension on this screen
image
  1. See if there are any errors in the console tab from clicking upon service worker.
  2. On the popup in chrome, click inspect and see if there are any errors in the devtools console tab

I'll try to replicate your issue by resetting any configs and stored values as if I was opening the extension for the first time.

@sstephanyy
Copy link
Contributor

sstephanyy commented May 21, 2024

Thanks for helping me out @cdrani

It's everything on.
image

When I click in the console tab from clicking upon service worker:
image

The erros in the devtools when I clicked inspect
image

image

@cdrani
Copy link
Owner Author

cdrani commented May 21, 2024

@sstephanyy: Okay. Thanks for the screenshots. It seems like image is not loading in the popup. When you load the live version from the web store / addon store is the same issues present?

Let me get a windows instance running and see if this is windows specific during development. I'll report back then. I am currently not sure without investigation further.

@cdrani
Copy link
Owner Author

cdrani commented May 21, 2024

@sstephanyy: I found the cause of the issue. It's quite a simple fix and since you discovered it and it's a blocker for you, would you be willing to create a PR for it? It's #217.

@sstephanyy
Copy link
Contributor

@cdrani : Hey, now that Window's problem was solved, I think I can keep working on this issue, right?

@cdrani
Copy link
Owner Author

cdrani commented May 23, 2024

@sstephanyy: Yup.

@sstephanyy
Copy link
Contributor

sstephanyy commented May 25, 2024

@cdrani hey, is there some way to inspect the modal, lik to see the console.log messages? I’d like to figure this out on my own to enhance my skills, so please keep the advice to a minimum. 😃

I'm trying to make like this:



#outsideClickModalListener() {
        window.addEventListener('click', async event => {
            const modal = document.getElementById('chorus-controls'); 
            if (event.target === modal) {
                await this.hide();
            }
        });
    }


@cdrani
Copy link
Owner Author

cdrani commented May 26, 2024

@sstephanyy: Are there console.logs in the modal? You can place logs in the event callback. The above looks like you are listening on the window, so you could use an if statement to log only events on the modal.

If you are on chrome, monitorEvents could be just as useful:

https://developer.chrome.com/blog/quickly-monitor-events-from-the-console-panel-2
https://briangrinstead.com/blog/chrome-developer-tools-monitorevents/

UPDATE

https://github.com/cdrani/chorus/blob/develop/src/models/chorus.js is the actual modal file. This model creates the modal and add's the ui inside of it. It also already has hide and show methods to work with

@sstephanyy
Copy link
Contributor

sstephanyy commented May 26, 2024

@cdrani : I'm feeling kinda stupid for not be able to figure this out, but I can't understand why the code can't detect clicks outside of the modal. Like, I put console.log on the code for debug purposes and the console.log('Click was ouside modall'); message is not being triggered, it suggests that the condition to detect clicks outside of the modal is not evaluating to true when it should... Can you give me a little help, please? tell me what I am doing wrong . I know if I remove the this.isShowing from the if statement, suddenly, my console.log('Click was ouside modall'); works

get isShowing() {
        if (!this.mainElement) return false

        return this.mainElement.style.display == 'block'
    }

    hide() {
        if (this.mainElement) {
            this.mainElement.style.display = 'none';
        }
    }
    
    #outsideClickListener() {
        const modal = document.getElementById('chorus-controls');

        document.addEventListener('click', (event) => {
            console.log('Click detected:', event.target);
            if (modal && this.isShowing && !modal.contains(event.target)) {
                this.hide()
                console.log('Click was ouside modal');
                
            }
        })
    }

image

@cdrani
Copy link
Owner Author

cdrani commented May 26, 2024

@sstephanyy: It looks like you are calling hide() as soon as you open the modal. You need to check that the target is not the actual button that triggers the modal opening.

When you click the settings button, the modal is set to be displayed, so it's showing AND the modal doesn't include the button that triggered its opening, so this.hide() is clicked. You need to update the if statement:

if (modal && this.isShowing && 
	!modal.contains(event.target) && 
	this.#checkTargetIsNotModalTriggerButton(event.target)
) {
}
image

I would suggest a method like #checkTargetIsNotModalTriggerButton because the target could be the button, the svg, or even the path. Maybe checking on the role attribute?

You are pretty close. 👍

@sstephanyy
Copy link
Contributor

@cdrani: Thank you so much for helping me! 😃 It seems that the hide() method is not being called, because the modal doesn't close as soon as I open it. The modal only closes if I click the "X" button inside of it, and that's where my problem lies, haha. When I click the button to open the modal, it does not trigger the click outside event.

I think this was happening when I made this version of code solution. In this solution, the modal was not being opened, because it was calling hide() as soon as I opened the modal:

#outsideClickListener() {
    const settingsbtn = document.getElementById('chorus-icon');

    document.addEventListener('click', (event) => {
        console.log('Clcked', event.target);

        const modal = document.getElementById('chorus-main');
        if (modal && !modal.contains(event.target) && event.target !== settingsbtn) {
            console.log('Clicked out of the modal');
            this.hide();
        }
    });
}

If is not asking too much, can you show me a little bit of how #checkTargetIsNotModalTriggerButton would be?

@cdrani
Copy link
Owner Author

cdrani commented May 27, 2024

@sstephanyy:

It seems that the hide() method is not being called, because the modal doesn't close as soon as I open it. [...] When I click the button to open the modal, it does not trigger the click outside event.

You code doesn't show where you actually register this click event. Notice in the init() where I call methods that register the event listeners? I assume though not shown, you call this.#outsideClickListener() inside of init?

init() {
    this.headerListeners = new HeaderListeners(this._songTracker)
    this.actionListeners = new ActionListeners(this._songTracker)
    this.#outsideClickListener() // need to register the event listener by calling this method
}

When we click on the settings button, the target captured might be the button, or its inner elements, as events bubble up. Regardless of if the button, the svg, or the path is clicked, a single click event is triggered and modal is toggled either on or off.

As for #isTargetSettingsButton method, it might be something like the pseudocode below:

#isTargetSettingsButton(target) {
  // We just need to check if the target has a role attribute
  // if it does, check if role is "settings".

  // if it does not, check if it's parentElement has a role attribute of "settings"
    
  // return false as the target either doesn't have role attribute 
  // and/or it's parentElement doesn't have role attribute of "settings"
}

Feel free to update the method name.

if (this.isShowing && !modal?contains(event.target) && !this.#isTargetSettingsButton(event.target)) {}

getAttribute
parentElement

Let me know if the pseudocode helps, otherwise I can be more explicit.

@sstephanyy
Copy link
Contributor

@cdrani: Thanks so much for the help with the pseudocode, I'm sure it will help me later 😃. I’m quite busy these days to work on the issue, but I'll come back to solve Thursday :)

@cdrani
Copy link
Owner Author

cdrani commented Jun 1, 2024

@sstephanyy: I went ahead and implemented this as I wanted to use this util in the new dropdown ui in the fx view. Thanks for your work in pushing it to this stage. It gave me a good point from which to continue. 👍

@sstephanyy
Copy link
Contributor

@cdrani: Oh man, sorry for leaving you hanging these past few days. I took a look at how you implemented the code to close the modal to learn from it since I was having some difficulty! 😆. I found the solution a little hard for me to understand and I don't think I would have been able to come up with that reasoning to implement this functionality as you did.

Is there any other issue I can work on to help you out?

@cdrani
Copy link
Owner Author

cdrani commented Jun 4, 2024

@sstephanyy: Think nothing of it. It's an OSS project that's I started when I wanted to to learn how to build a web extension. I also only work on it when I can or encounter a serious bug. In fact I just came back from a 5 month break only about a month ago.

My solution was based on something I implemented on a another project. It's based on svelte directives. I wouldn't have come across that reasoning either without first using svelte a month or two ago. Your solution would have worked just as well, however it's modal specific, and would have required a small refactor to reuse the base logic for other elements. We would have reached a similar solution, but the update to the reverb presets required this functionality sooner than later.

Most issues are refactors or new features. I could give you a list to pick from, such as:

  1. Add mute button and volume setter in popup
  2. Add current song position & song endtime in popup
  3. settings toggle in popup to toggle reverb presets, blocked songs, etc ,as currently it's either all features on or off.
  4. A view to display songs that are marked as snips or blocked. User can then play exclusively snips or unblock blocked tracks. I envision this to either be in the page like the queue view, or a sidepanel for easy access from any tab.
  5. Show the popup for ~5 seconds on song change it not currently open. This would be a setting a user can opt-in.

There's a lot more, but if any of the above sound interesting, or if you have one in mind you want to implement, feel free to take it up. I can provide guidance where I can.

Off topic, but I am thinking of starting another project in the same vein. It has a singular focus of getting new releases based on a user's saved artists and/or artists on tracks in their Liked Songs playlist. Tracks could be added to a weekly playlist. A user could subscribe/unsubcribe to an artist. Maybe notifications are sent for new releases? I am thinking it will a web extension and desktop sharing the same/similar ui and logic. I will most likely use svelte + tauri.

Let me know if you are interested in either or both. None is also fine. 😄

@sstephanyy
Copy link
Contributor

@cdrani: Helloooo. You scared me. I thought you wouldn't talk to me anymore 😄.

Thank you for sharing these suggestions for me! I’m inclined to work on the snips download feature (that one I told you). This would allow users to download specific snippets of songs, which is a unique functionality and would be really useful for me when using the Chrous extension.

Yeah, I think your idea of the new project sounds fascinating. But I'm not familiar with Svelte and Tauri (I even had to search what Tauri was 😆). As a beginner, I find myself needing to focus on mastering one language at a time before venturing into others. So I can't really help on your new project, because I'm worried I might just get in the way rather than help you out. Is there any project involving JavaScript frameworks like React or Angular, or even backend languages like Python or C# (I'm not an expert in any of this. But at least I already had contact with them) that you use and you wanna help with it?

But I'm really enjoying contributing to Chorus. Having you review my code and teach me has been really cool 😄.

@cdrani
Copy link
Owner Author

cdrani commented Jun 6, 2024

@sstephanyy: Ahah. I was just a little sick.

I would love to help build the snip download feature, but I am wary of introducing it into the extension. I assume it would violate Spotify's TOS and I would not want to impose that on users without their acknowledgment. It might also be telling that the web store doesn't have similar extensions for downloading Spotify audio or youtube videos. It still possible to do this locally using something zotify and piping the file to ffmpeg to trim it based on snip values.

As a beginner, I find myself needing to focus on mastering one language at a time before venturing into others.

I understand this sentiment. I don't mind using React instead of Svelte. At the end of the day it's We will still just be writing JS code. However, the first part of the project would just be the chrome extension, which would touch on using chrome apis like storage, notifications, and alarms. The Tauri part will just be used for setting the app window sizes, and packaging it to work on windows, linux, macos, and ios/android. It will still be mostly JS.

Is there any project involving JavaScript frameworks like React or Angular
react-image-effects is an old project that could be revamped. We could rewrite it as an extension. User could click on an effect, have the effect applied to the image, and download the image with the effect applied. Or maybe rewrite it in angular? 🤷 If you have an interesting project idea we could also work on that. I have dabbled in angular and angular years ago, but I wouldn't mind picking it up again.

I'm worried I might just get in the way rather than help you out.
I don't want you to think of it as my project and you are a contributor. Think of it as we are both contributors of the project - I just started earlier. We are both learning, differently but equally.

@sstephanyy
Copy link
Contributor

sstephanyy commented Jun 6, 2024

I hope you are better now 😄.

You’re correct to be cautious about introducing a snip download feature for Spotify content. It's such a pity, because would be a great feature for the users 😢 .

If you don't mind using React instead of Svelte, create a new repository of the project because I'm all in. I realize you really like to make Chrome extensions haha. A time ago, I made an extension to help me to learn English, It enabled users to capture words from any webpage and compile them into a list. This list provided word definitions and authentic examples of usage, particularly from movie and TV series scenes. I didnt publish it, because I had to pay to put in the Chrome store haha.

I once had an idea of project. We’re often caught up in endless social media scrolling, right? Imagine this: after watching 10 for example, Instagram reels, TikTok videos, or YouTube shorts, the extension automatically redirects you to a quiz related to a subject you’re studying. It’s like a fun interruption that nudges you back to your academic goals. Do you think it has potential? I am divided if it is a good idea, because most users from these social media prefer to use their phone to use these apps and the chrome extension wouldn't work inide the app....

Or maybe you know what we can build? we can build a program to make videos for us on this social media and then we would earn money with these videos hahahaha. I know, it is a stupid idea, but I think it has potential, we would just make a video script + narrative voice + images in the background with few transitions and we are done haha.

Something like this, I don't know yet for sure. But we can use it as a source of inspiration.
https://www.youtube.com/watch?v=Mu6hloC7ty8

My humble extension 😆
image

Do you have an account on Discord? We could talk about our projects ideas there :)

@cdrani
Copy link
Owner Author

cdrani commented Jun 7, 2024

@sstephanyy: Yup. It sucks being sick during summer.

I will create it in an organization and invite you. Chrome extensions are an untapped space and going from ideation to release is a quick cycle.

Your written english is amazing, so looks like your extension helped. You should release it on the store. For chrome webstore it's $5 USD to upload 20 extensions. FireFox and Edge is free.

It might be a good idea, but still requires willpower on the user. Scrolling on social media is addicting enough that even with measures like ScreenTime, people can just dismiss it or come back to the app. They would need another incentive to leave/pause scrolling and focus on something else.

we can build a program to make videos for us on this social media and then we would earn money with these videos hahahaha.

I came across this repo back a week or so: steam-reviews-tiktok. This could be a good starting point for shorts or reels.

Discord username is c.drani

Repository owner locked as resolved and limited conversation to collaborators Jun 7, 2024
Repository owner unlocked this conversation Jun 10, 2024
@sstephanyy
Copy link
Contributor

sstephanyy commented Jun 10, 2024

@cdrani: I sent you a friend request on Discord a few days ago. My username is sstephanyyy

Repository owner locked as resolved and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants