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: close dialogs when clicking outside #999

Merged
merged 14 commits into from
Mar 22, 2023

Conversation

takanome-dev
Copy link
Contributor

@takanome-dev takanome-dev commented Mar 13, 2023

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Description

This pr fixes the issues with dialogs that won't close after clicking outside of the component.

  • If the dialog has to be controlled, two props must be passed to the radix portal so that it will close the dialog if the outside is clicked. Those props are open and onPageChange
  • Note that for deleting a highlight, we're using radix alert which won't close when clicking outside.
  • Unrelated but the dialog showing a single highlight is too small on my end.

Related Tickets & Documents

Fixes #985

Mobile & Desktop Screenshots/Recordings

https://www.loom.com/share/32833c5e8fd44d73b90cf9c607461304

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

@netlify
Copy link

netlify bot commented Mar 13, 2023

βœ… Deploy Preview for oss-insights ready!

Built without sensitive environment variables

Name Link
πŸ”¨ Latest commit d7297b0
πŸ” Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/641b351d3af66c0008016d12
😎 Deploy Preview https://deploy-preview-999--oss-insights.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 site settings.

@netlify
Copy link

netlify bot commented Mar 13, 2023

βœ… Deploy Preview for design-insights ready!

Name Link
πŸ”¨ Latest commit d7297b0
πŸ” Latest deploy log https://app.netlify.com/sites/design-insights/deploys/641b351dc35b8200099eeea2
😎 Deploy Preview https://deploy-preview-999--design-insights.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 site settings.

@takanome-dev takanome-dev marked this pull request as ready for review March 14, 2023 09:38
@brandonroberts
Copy link
Contributor

@takanome-dev this works if I click a link on the page to open the modal, but now it doesn't open the modal when navigating directly.

Screen.Recording.2023-03-14.at.9.06.08.AM.mov

@takanome-dev

This comment was marked as resolved.

Copy link
Contributor

@0-vortex 0-vortex left a comment

Choose a reason for hiding this comment

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

You need to update your branch with beta to trigger the latest release code and return or await the router redirects you implemented in the feed in order to be able to debug them correctly.

It's likely this is 95% ready, just need to get rid of that error πŸ‘

pages/feed/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

PR Compliance Checks Passed!

@brandonroberts
Copy link
Contributor

@takanome-dev still seeing the same issue with the modal closing on page load

@takanome-dev
Copy link
Contributor Author

@takanome-dev still seeing the same issue with the modal closing on page load

Yeah, honestly I don't know how to fix it yet, the component re-renders multiple times that’s why it closes. I will investigate more on it, any help is appreciated.

@brandonroberts
Copy link
Contributor

@takanome-dev still seeing the same issue with the modal closing on page load

Yeah, honestly I don't know how to fix it yet, the component re-renders multiple times that’s why it closes. I will investigate more on it, any help is appreciated.

Sure thing. I'll take a look soon

@takanome-dev
Copy link
Contributor Author

@brandonroberts it should be good now

@OgDev-01
Copy link
Contributor

OgDev-01 commented Mar 22, 2023

The new fix introduces another bug

Screen.Recording.2023-03-22.at.07.13.28.mov

viewing a single highlight is now blocked when a filter is selected selected.

@OgDev-01
Copy link
Contributor

Works as expected now πŸ•

@takanome-dev
Copy link
Contributor Author

Works as expected now pizza

Not yet πŸ˜… When removing the filter, it still stays in the URL I think
πŸ˜…

@takanome-dev takanome-dev added needs review PRs that need review from core engineering team and removed requested changes labels Mar 22, 2023

<div>
<a href={prLink} className="underline text-sauced-orange cursor-pointer">
Copy link
Contributor

Choose a reason for hiding this comment

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

@bdougie, should this be changed to a regular p element or given the target="_blank" attribute to open the link in a new tab?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding target=_blank would be good

Copy link
Contributor

@brandonroberts brandonroberts left a comment

Choose a reason for hiding this comment

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

Works as expected

@brandonroberts
Copy link
Contributor

brandonroberts commented Mar 22, 2023

@takanome-dev after you resolve the merge conflicts, then we're ready to merge

@takanome-dev takanome-dev added ready-to-merge and removed needs review PRs that need review from core engineering team labels Mar 22, 2023
@brandonroberts brandonroberts merged commit 1bc90bb into open-sauced:beta Mar 22, 2023
github-actions bot pushed a commit that referenced this pull request Mar 22, 2023
## [1.37.0-beta.3](v1.37.0-beta.2...v1.37.0-beta.3) (2023-03-22)

### πŸ› Bug Fixes

* close highlight dialog when clicking outside its container ([#999](#999)) ([1bc90bb](1bc90bb))
@github-actions
Copy link
Contributor

πŸŽ‰ This PR is included in version 1.37.0-beta.3 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

@takanome-dev takanome-dev deleted the 985-dialog-bug branch March 22, 2023 17:21
github-actions bot pushed a commit that referenced this pull request Mar 24, 2023
## [1.37.0](v1.36.0...v1.37.0) (2023-03-24)

### πŸ§‘β€πŸ’» Code Refactoring

* Deleted unused hooks, removed unused exports and imports ([#1028](#1028)) ([7b2bda2](7b2bda2))

### πŸ• Features

* add teams UI for the design system ([#994](#994)) ([499d08e](499d08e))
* added active styles to navbar links ([#1016](#1016)) ([c25c0a3](c25c0a3)), closes [#697](#697)
* create collaboration Ui components ([#1006](#1006)) ([19ff5a7](19ff5a7))
* replace react toast to radix ([#933](#933)) ([3e000be](3e000be))
* update top nav menu to be more responsive on mobile ([#1029](#1029)) ([90c1e4a](90c1e4a)), closes [#866](#866)

### πŸ› Bug Fixes

*  updated footer file to update the year dynamically ([#1040](#1040)) ([c766d95](c766d95))
* add a check for proper URL in avatarURL prop ([#1009](#1009)) ([a425279](a425279))
* center text inside upgrade access button ([#1027](#1027)) ([cf3f6cb](cf3f6cb)), closes [#1024](#1024)
* close highlight dialog when clicking outside its container ([#999](#999)) ([1bc90bb](1bc90bb))
* make delete standout ([#1026](#1026)) ([9c68287](9c68287))
* make insights hub layout inconsistent on larger screens ([#1046](#1046)) ([daaf6d1](daaf6d1))
* refresh highlights list on update and delete ([#1034](#1034)) ([bf2d951](bf2d951))
* remove dialog layout overlap on single highlight view ([#1047](#1047)) ([ea836e8](ea836e8))
* update avatar style to remain aligned with the input field ([#1037](#1037)) ([2374f20](2374f20))
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.37.0-beta.3](open-sauced/app@v1.37.0-beta.2...v1.37.0-beta.3) (2023-03-22)

### πŸ› Bug Fixes

* close highlight dialog when clicking outside its container ([#999](open-sauced/app#999)) ([1bc90bb](open-sauced/app@1bc90bb))
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.37.0](open-sauced/app@v1.36.0...v1.37.0) (2023-03-24)

### πŸ§‘β€πŸ’» Code Refactoring

* Deleted unused hooks, removed unused exports and imports ([#1028](open-sauced/app#1028)) ([7b2bda2](open-sauced/app@7b2bda2))

### πŸ• Features

* add teams UI for the design system ([#994](open-sauced/app#994)) ([499d08e](open-sauced/app@499d08e))
* added active styles to navbar links ([#1016](open-sauced/app#1016)) ([c25c0a3](open-sauced/app@c25c0a3)), closes [#697](open-sauced/app#697)
* create collaboration Ui components ([#1006](open-sauced/app#1006)) ([19ff5a7](open-sauced/app@19ff5a7))
* replace react toast to radix ([#933](open-sauced/app#933)) ([3e000be](open-sauced/app@3e000be))
* update top nav menu to be more responsive on mobile ([#1029](open-sauced/app#1029)) ([90c1e4a](open-sauced/app@90c1e4a)), closes [#866](open-sauced/app#866)

### πŸ› Bug Fixes

*  updated footer file to update the year dynamically ([#1040](open-sauced/app#1040)) ([c766d95](open-sauced/app@c766d95))
* add a check for proper URL in avatarURL prop ([#1009](open-sauced/app#1009)) ([a425279](open-sauced/app@a425279))
* center text inside upgrade access button ([#1027](open-sauced/app#1027)) ([cf3f6cb](open-sauced/app@cf3f6cb)), closes [#1024](open-sauced/app#1024)
* close highlight dialog when clicking outside its container ([#999](open-sauced/app#999)) ([1bc90bb](open-sauced/app@1bc90bb))
* make delete standout ([#1026](open-sauced/app#1026)) ([9c68287](open-sauced/app@9c68287))
* make insights hub layout inconsistent on larger screens ([#1046](open-sauced/app#1046)) ([daaf6d1](open-sauced/app@daaf6d1))
* refresh highlights list on update and delete ([#1034](open-sauced/app#1034)) ([bf2d951](open-sauced/app@bf2d951))
* remove dialog layout overlap on single highlight view ([#1047](open-sauced/app#1047)) ([ea836e8](open-sauced/app@ea836e8))
* update avatar style to remain aligned with the input field ([#1037](open-sauced/app#1037)) ([2374f20](open-sauced/app@2374f20))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: close update dialog disables mouse event on page body
4 participants