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

Fix context menu on modals #2154

Merged
merged 8 commits into from
May 30, 2022
Merged

Fix context menu on modals #2154

merged 8 commits into from
May 30, 2022

Conversation

tanmoyAtb
Copy link
Contributor

@tanmoyAtb tanmoyAtb commented May 25, 2022

With the addition of context menu, Right click on any modal that is inside our file browser was opening the context menu.

This PR fixes the issue.
The simple fix was to move the modals out of the article component that responds to right click event.

@render
Copy link

render bot commented May 25, 2022

@render
Copy link

render bot commented May 25, 2022

@render
Copy link

render bot commented May 25, 2022

@github-actions github-actions bot added the Type: Bug Fix Added to PRs if they are addressing a bug label May 25, 2022
@asnaith
Copy link
Member

asnaith commented May 25, 2022

Looks good on the modals now (these right-click changes are so awesome btw!).

I noticed the same issue happens on the home page buttons too. Not sure if you want to apply the fix for that in this PR or should we create a new ticket?

Screen.Recording.2022-05-25.at.10.35.20.AM.mov

@juans-chainsafe
Copy link
Contributor

Awesome! nice, I agree with Andrew, I think the right click should only trigger the context menu INSIDE the table, if we can add that will be great

@tanmoyAtb
Copy link
Contributor Author

Looks good on the modals now (these right-click changes are so awesome btw!).

I noticed the same issue happens on the home page buttons too. Not sure if you want to apply the fix for that in this PR or should we create a new ticket?

Screen.Recording.2022-05-25.at.10.35.20.AM.mov

definitely in this PR, looking into this.

@tanmoyAtb
Copy link
Contributor Author

All right. I've prevented right click menu on
The breadcrumbs
The action buttons in the header.
I think this should cover where the context menu should be prevented.

@juans-chainsafe
Copy link
Contributor

All right. I've prevented right click menu on The breadcrumbs The action buttons in the header. I think this should cover where the context menu should be prevented.

I can't see a commit for this, did you push the changes? also I checked the UI but is still the same as before

@tanmoyAtb
Copy link
Contributor Author

All right. I've prevented right click menu on The breadcrumbs The action buttons in the header. I think this should cover where the context menu should be prevented.

I can't see a commit for this, did you push the changes? also I checked the UI but is still the same as before

My bad. I pushed the changes now.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Some conflicts snuck in, but I could review and it's working well from my test 💯

Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

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

Working better now, thanks for the updates! I think once the PR conflict is resolve we can merge it

@tanmoyAtb tanmoyAtb enabled auto-merge (squash) May 30, 2022 15:08
@tanmoyAtb tanmoyAtb merged commit ea789cc into dev May 30, 2022
@tanmoyAtb tanmoyAtb deleted the fix/context-menu-on-modals branch May 30, 2022 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Fix Added to PRs if they are addressing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants