Multiple popups open up at a time #793

RaviAnand111 · 2022-01-15T06:48:03Z

fixes #750

This is the code I have done, all the improvement I had done till yesterday, I have deleted it this code just has the code I talked about in the issue chats, now we can discuss and find a way to solve this problem.
Thank You
@NARUDESIGNS @TildaDares

gitpod-io · 2022-01-15T06:48:06Z

NARUDESIGNS · 2022-01-17T13:31:19Z

@RaviAnand111 I fixed this but with more lines of code. I noticed that even bootstrap's example couldn't close when you click again on the button but only closes when you click outside it. So I couldn't find a resolution through bootstrap which made me write more lines of code.
I will create a PR now so you can see my approach and maybe we can get @TildaDares's input to know if it is okay to follow the pattern to resolve for other popovers.

NARUDESIGNS · 2022-01-17T13:32:51Z

Or would you prefer me to share with you the code snippet I added so that you can try it on your local environment and then push the changes?
@RaviAnand111

RaviAnand111 · 2022-01-17T15:24:08Z

Both are fine @NARUDESIGNS you can share or make a PR whichever sounds good to you, as soon as you push it can we try to find ways to solve this.
Thanks

TildaDares · 2022-01-17T16:17:45Z

Hi @NARUDESIGNS, to make it easier for @RaviAnand111 to incorporate those changes you can make use of GitHub suggestions.

NARUDESIGNS · 2022-01-17T21:27:14Z

Thank you @TildaDares but his commits didn't include lines where I wanted to indicate the changes. I thought of making a PR cause I don't know if this can explain it enough but let me try this first.
@RaviAnand111 Please have a look at these snippets and then try to insert them. I've included some codes that were there before so that you can have an idea of where to include these snippets. The file is PublicLab.RichTextModule.Table.js

+ let popoverIsOpen = false;
  $('.wk-commands .woofmark-command-table').click(function() {
+    popoverIsOpen = !popoverIsOpen;
    $('.ple-table-size').click(function() {

        $('.woofmark-command-table').popover('toggle');
      });
    });

+    // close table popover when user click outside the page
+   $(':not(".woofmark-command-table")').click((e) => {
+      // check to see that the clicked element isn't fa-table icon, else when you click on the fa-table icon, the popover will close
+      if($('.woofmark-command-table').children()[0] != e.target){
+        const popoverContainer = document.querySelector('.popover');
+        const isChildElement = popoverContainer.contains(e.target);
+        if (popoverIsOpen && !e.target.classList.contains("woofmark-command-table") && !isChildElement) {
+          $('.woofmark-command-table').click();
+        }
+      }
+    });
  });
};

NARUDESIGNS · 2022-01-17T21:43:46Z

I hope this helps.

RaviAnand111 · 2022-01-18T04:32:06Z

Thanks @NARUDESIGNS , I will add them in the code and make a commit here.

RaviAnand111 · 2022-01-18T06:31:23Z

Hey @NARUDESIGNS, you have written a very beautifull code, if is working perfectly.

NARUDESIGNS · 2022-01-18T07:16:36Z

Thank you @RaviAnand111 even though I feel like it's many lines of code for a simple task but you get the idea or pattern we can use for the other popovers right?

we use a boolean popoverIsOpen variable to know whether or not the popover is open.
next, we check if there's a click on the page and not the table tool button or its icon
we then check to see if the element being clicked is contained in the popover because we wouldn't want to hide it if, for example, the user clicks on the - or + button
if all checks out, then we just want to call the click event on the table button which is capable of hiding the popover.

RaviAnand111 · 2022-01-18T07:31:08Z

I got it, @NARUDESIGNS
Thanks for your help, I will try to learn as much as I can from you.
And now I will add this code in every button in .wk-commands and then commit it.
Thanks again
@NARUDESIGNS @TildaDares for this great opportunity where I am getting alot to learn.

NARUDESIGNS · 2022-01-18T07:55:06Z

Alright. At Public Lab, I learned that a PR should do just one thing (unless there are very minimal changes you want to group together). So I think we should make separate PRs for each tool button that has popovers. You can make changes for this table button and commit it then create another PR for any other tool button you'd like to work on next.

RaviAnand111 · 2022-01-18T10:05:48Z

ok great, then I will do it the way you said, and different PRs for different popovers.
Thanks
@NARUDESIGNS @TildaDares

This reverts commit e350928.

This reverts commit b43cfbe.

RaviAnand111 · 2022-01-18T11:31:23Z

I just pushed the new changes, you can check and tell me if there is something to change.
So now I have to make a new PR and solve popup problem of other buttons just like this,
So I will be doing that.
Thanks
@NARUDESIGNS

RaviAnand111 · 2022-01-18T11:45:08Z

I just had this doubt, @NARUDESIGNS as in the end all the code in modules become part of PublicLab.Editor.js so I cannot create same variables again again right, I would have to give different name to every variable in every module.
right ?

NARUDESIGNS · 2022-01-18T11:50:04Z

The let and const declaration keywords scopes the variable to its enclosing function, therefore the variable isn't global but is only accessible within the function it was declared.
I don't think this will be a problem but when you make your next attempt we can find out.

RaviAnand111 · 2022-01-18T12:13:37Z

I tried both ways, with different variable names and with same variable names and I found out that both ways are working perfectly, there is not problem, so what do you recommend which way should I do it ?
Thanks

NARUDESIGNS · 2022-01-18T12:32:14Z

Well for sake of consistency, let's just use same one for all.

RaviAnand111 · 2022-01-18T13:10:50Z

I am adding the function in try catch as it is printing errors in console on clicking on the page as the contains function is being called on null.

NARUDESIGNS · 2022-01-18T13:57:25Z

Is this for a different tool button and popover?
If yes, I think we should make a PR for it, and then we can move the conversation about it there.

RaviAnand111 · 2022-01-18T14:11:11Z

See this is the error in the console,
image

what I have understood is,

    $(':not(".woofmark-command-table")').click((e) => {
     // check to see that the clicked element isn't fa-table icon, else when you click on the fa-table icon, the popover will close
     if($('.woofmark-command-table').children()[0] != e.target){
       const popoverContainer = document.querySelector('.popover');
       const isChildElement = popoverContainer.contains(e.target);
       if (popoverIsOpen && !e.target.classList.contains("woofmark-command-table") && !isChildElement) {
         $('.woofmark-command-table').click();
       }
     }
    });```

as we are calling .contains so it is giving error on being called on null

NARUDESIGNS · 2022-01-18T16:06:00Z

Yea I noticed. I dropped a suggestion for you. Please check.
Please try adding the changes and see if it eliminates the error. I was able to reproduce the error and that fixed it for me.
@RaviAnand111

publiclab#793

RaviAnand111 · 2022-01-18T16:39:04Z

hey @NARUDESIGNS , sorry to ask, where can I see the suggestion, I can't find any.
Thanks

NARUDESIGNS · 2022-01-18T17:09:20Z

Yea there are too many comments already so I'd just paste it here:

if (popoverIsOPen && $('.woofmark-command-table').children()[0] != e.target) {

RaviAnand111 · 2022-01-19T09:20:19Z

I am adding the same video from the other PR, if any more demo video is required, then I will make another video

multiple-popover-bug.mp4

You can check the demo video
@NARUDESIGNS @TildaDares @jywarren
Thanks

NARUDESIGNS

Awesome!

src/modules/PublicLab.RichTextModule.Table.js

NARUDESIGNS · 2022-01-19T10:14:01Z

Hi, @RaviAnand111 there are 10 commits in this PR and some of the commits are from another PR. This usually happens when you checkout a new branch X from another branch Y, the commits from branch Y are automatically added in new branch X.

Did you checkout this branch from main branch?

RaviAnand111 · 2022-01-19T10:26:02Z

I think I have made a mistake here, and checkout a new branch from a branch,
What can I do to solve this ? 😨

NARUDESIGNS · 2022-01-19T10:46:35Z

Not to worry, I made same mistake some time when I joined as well. Tilda shared a very helpful resource, have a look at it here - https://luisdalmolin.dev/blog/branched-off-the-wrong-branch-in-git/

RaviAnand111 · 2022-01-19T15:32:24Z

hey @NARUDESIGNS I think I have done a massive mistake here.

Can you tell me how to solve, I am confused right now

NARUDESIGNS · 2022-01-19T16:18:42Z

I'm sorry, I don't really get your diagram but all the commits here are still in place. Maybe what you can do is git reset --hard <commit> where <commit> is the latest commit in your RaviAnand111:multiple-popups branch before you took these actions. This will reset all commits and update your local environment to point to the reverted commit. I think the <commit> should be this last one: c483f01

@TildaDares can you please help to check in that we are on track to resolve the issue @RaviAnand111 is having, thank you.

RaviAnand111 · 2022-01-19T17:22:41Z

My mistakes:

I made a PR from main branch Padding added to table popover button for editor fixes#752 #768 Padding added to table popover button for editor fixes#752 #769
then I created a branch multiple-popupsfrom main, there made a PR too Multiple popups open up at a time #793
then created another branch from multiple-popups branch with name customInsert-Popover-problem and made a PR from there too. Custom insert popover problem Solved #794

It seems messed up, what should I do ?

TildaDares · 2022-01-19T17:40:13Z

Not to worry, I made same mistake some time when I joined as well. Tilda shared a very helpful resource, have a look at it here - https://luisdalmolin.dev/blog/branched-off-the-wrong-branch-in-git/

@RaviAnand111 Have you tried this?

RaviAnand111 · 2022-01-19T17:41:24Z

Yeah, I tried this, it is giving merge conflict and something, I think the main mistake I did was, made a change and PR from main branch @TildaDares

RaviAnand111 · 2022-01-19T17:47:57Z

I think I should create a new branch, before I created a PR from main and git rebase that PR to that branch, what do you guys think ? is there any way to do that safely ?
@NARUDESIGNS @TildaDares

TildaDares · 2022-01-19T17:50:40Z

@RaviAnand111 It’s best to leave the main branch untouched and use branches for your work.

I’m looking for resources to help with your issue.

RaviAnand111 · 2022-01-19T17:59:30Z

Yes I learned my lesson from this mistake, thankyou for helping me,

TildaDares · 2022-01-19T17:59:59Z

@RaviAnand111 I found this article about how to rebase. You could pick and drop the commute you want in this PR. https://www.google.com/amp/s/www.digitalocean.com/community/tutorials/how-to-rebase-and-update-a-pull-request.amp

RaviAnand111 · 2022-01-19T20:52:00Z

Hey guys, I tried to solve this so I will write down what steps I took :

I created a branch feature at the commit before the commit in which I made a PR from main.
Then I did git rebase from the main commit in which I did a PR and transported all of commits after it in the feature branch.
Then I did git reset to the commit point in which I created feature branch.
Then I did a force push git push -f to my remote repo.
Then I did git fetch upstream and git pull upstream main

I think I should make a video with sketch and drawing to make you guys understand, if you guys ask for it then I will make one.

so now we are here, now head is at the commit in which I have not pushed a PR yet, but the PR's I have done already are showing those previous files.

So I am thinking of closing those PR's and creating new one's
It was alot confusing but I tried to solve it so you guys don't have to waste time on it but I don't know if it was right or wrong
Its 2:30 AM here, now I am going to sleep,
If I did any mistake then please share and thankyou for helping

NARUDESIGNS · 2022-01-20T11:28:30Z

Hello @RaviAnand111
Though your RaviAnand111:main branch is now in sync with publiclab:main, this PR still contains 10 commits, with the first 2 commits about #768

Maybe we need to just start the whole process all over since your main branch is now properly synced with that of publiclab.
So I advise that you

check out to main (i.e git checkout main)
create and branch off to a new branch (e.g git checkout -b "fix-multiple-popover-table-popover")
make changes and push new branch/changes to remote repo
create new PR

How about that?
cc @TildaDares

RaviAnand111 · 2022-01-20T11:39:08Z

That sounds good @NARUDESIGNS ,
once @TildaDares confirms that, I will start doing it.

RaviAnand111 · 2022-01-20T11:39:29Z

And one more thing,
should I delete the PR's I have created already ?

TildaDares · 2022-01-20T11:44:55Z

Hi @RaviAnand111, you can follow those steps @NARUDESIGNS outlined. You can also close these PRs when you’ve opened the new ones. Thanks!

RaviAnand111 · 2022-01-20T11:45:18Z

Thanks @NARUDESIGNS @TildaDares

RaviAnand111 and others added 5 commits December 30, 2021 14:36

Padding added to table popover button for editor fixes#752 publiclab#768

4910c9a

Padding added to table popover button for editor fixes#752 publiclab#768

c0a1e3a

Merge branch 'publiclab:main' into main

ebda258

Merge branch 'publiclab:main' into main

3961701

Multiple PopUps open at a time fixes#750

b43cfbe

RaviAnand111 added 4 commits January 18, 2022 16:19

Solved the problem with multiple popups at a time

e350928

Revert "Solved the problem with multiple popups at a time"

9501870

This reverts commit e350928.

Revert " Multiple PopUps open at a time fixes#750"

4730f8c

This reverts commit b43cfbe.

Solved multiple popups at a time fixes publiclab#750 publiclab#793

db4df0d

RaviAnand111 added a commit to RaviAnand111/PublicLab.Editor that referenced this pull request Jan 18, 2022

CustomInsert in Command Table popover problem solved fixes publiclab#750

4626a4a

publiclab#793

RaviAnand111 mentioned this pull request Jan 18, 2022

Custom insert popover problem Solved #794

Closed

Table button popover console error solved

c483f01

NARUDESIGNS reviewed Jan 19, 2022

View reviewed changes

src/modules/PublicLab.RichTextModule.Table.js Outdated Show resolved Hide resolved

RaviAnand111 closed this Jan 21, 2022

RaviAnand111 deleted the multiple-popups branch January 21, 2022 08:38

This was referenced Jan 21, 2022

Table popup hide on click on page and everything except table button … #800

Merged

Custom Insert popover hide on clicking on anything on the screen fixe… #809

Merged

Multiple popups open up at a time #793

Multiple popups open up at a time #793

Conversation

RaviAnand111 commented Jan 15, 2022

gitpod-io bot commented Jan 15, 2022

NARUDESIGNS commented Jan 17, 2022

NARUDESIGNS commented Jan 17, 2022

RaviAnand111 commented Jan 17, 2022

TildaDares commented Jan 17, 2022

NARUDESIGNS commented Jan 17, 2022

NARUDESIGNS commented Jan 17, 2022

RaviAnand111 commented Jan 18, 2022

RaviAnand111 commented Jan 18, 2022

NARUDESIGNS commented Jan 18, 2022 • edited Loading

RaviAnand111 commented Jan 18, 2022

NARUDESIGNS commented Jan 18, 2022

RaviAnand111 commented Jan 18, 2022

RaviAnand111 commented Jan 18, 2022

RaviAnand111 commented Jan 18, 2022

NARUDESIGNS commented Jan 18, 2022 • edited Loading

RaviAnand111 commented Jan 18, 2022

NARUDESIGNS commented Jan 18, 2022

RaviAnand111 commented Jan 18, 2022

NARUDESIGNS commented Jan 18, 2022

RaviAnand111 commented Jan 18, 2022

NARUDESIGNS commented Jan 18, 2022 • edited Loading

RaviAnand111 commented Jan 18, 2022

NARUDESIGNS commented Jan 18, 2022

RaviAnand111 commented Jan 19, 2022

NARUDESIGNS left a comment

Choose a reason for hiding this comment

NARUDESIGNS commented Jan 19, 2022

RaviAnand111 commented Jan 19, 2022

NARUDESIGNS commented Jan 19, 2022

RaviAnand111 commented Jan 19, 2022

NARUDESIGNS commented Jan 19, 2022

RaviAnand111 commented Jan 19, 2022

TildaDares commented Jan 19, 2022

RaviAnand111 commented Jan 19, 2022

RaviAnand111 commented Jan 19, 2022

TildaDares commented Jan 19, 2022

RaviAnand111 commented Jan 19, 2022

TildaDares commented Jan 19, 2022

RaviAnand111 commented Jan 19, 2022

NARUDESIGNS commented Jan 20, 2022 • edited Loading

RaviAnand111 commented Jan 20, 2022

RaviAnand111 commented Jan 20, 2022

TildaDares commented Jan 20, 2022

RaviAnand111 commented Jan 20, 2022

NARUDESIGNS commented Jan 18, 2022 •

edited

Loading

NARUDESIGNS commented Jan 18, 2022 •

edited

Loading

NARUDESIGNS commented Jan 18, 2022 •

edited

Loading

NARUDESIGNS commented Jan 20, 2022 •

edited

Loading