Skip to content

adrienne / added close icon and onclickoutside handler for modals in wallet #10203

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.

Sign up for GitHub

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

Jump to bottom
Merged
yashim-deriv merged 3 commits into from
Sep 21, 2023
Merged

adrienne / added close icon and onclickoutside handler for modals in wallet #10203

yashim-deriv merged 3 commits into from
Sep 21, 2023

Conversation

@adrienne-deriv
Copy link
Contributor

@adrienne-deriv adrienne-deriv commented Sep 21, 2023

Changes:

Please provide a summary of the change.

Screenshots:

Please provide some screenshots of the change.

@vercel
Copy link

vercel bot commented Sep 21, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
deriv-app ✅ Ready (Inspect) Visit Preview Sep 21, 2023 6:52am

adrienne-deriv
adrienne-deriv commented Sep 21, 2023
View reviewed changes
packages/wallets/src/components/WalletModal/WalletModal.tsx Outdated
const { hide } = useModal();
const modalRef = useRef(null);

useOnClickOutside(modalRef, should_close_on_click_outside ? hide : () => undefined);
Copy link
Contributor Author

@adrienne-deriv adrienne-deriv Sep 21, 2023

Choose a reason for hiding this comment

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

eslint was complaining that i cannot put empty function () => {}, so I put () => undefined instead 😢

Copy link
Contributor

@hamza-deriv hamza-deriv Sep 21, 2023

Choose a reason for hiding this comment

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

I think it is becasue of esLint rule
The silent error handling that it should explicitly return something instead of just inferring at runtime.
cc:@farzin-deriv

@adrienne-deriv adrienne-deriv marked this pull request as ready for review September 21, 2023 04:24
@github-actions
Copy link
Contributor

github-actions bot commented Sep 21, 2023 •
edited
Loading

A production App ID was automatically generated for this PR. (log)

Click here to copy & paste above information. 
- **PR**: [https://github.com/binary-com/deriv-app/pull/10203](https://github.com/binary-com/deriv-app/pull/10203)
- **URLs**:
    - **w/ App ID + Server**: https://deriv-app-git-fork-adrienne-deriv-wallets-modal-wrapper.binary.sx?qa_server=red.binaryws.com&app_id=32187
    - **Original**: https://deriv-app-git-fork-adrienne-deriv-wallets-modal-wrapper.binary.sx
- **App ID**: `32187`

@github-actions
Copy link
Contributor

github-actions bot commented Sep 21, 2023 •
edited
Loading

🚨 Lighthouse report for the changes in this PR:

Category Score
🔺 Performance 16
🟧 Accessibility 75
🟢 Best practices 92
🟧 SEO 85
🟢 PWA 90

Lighthouse ran with https://deriv-app-git-fork-adrienne-deriv-wallets-modal-wrapper.binary.sx/

@coveralls
Copy link

coveralls commented Sep 21, 2023

Coverage Status

Changes unknown when pulling 7cf9d67 on adrienne-deriv:wallets-modal-wrapper into ** on binary-com:master**.

hamza-deriv
hamza-deriv reviewed Sep 21, 2023
View reviewed changes
packages/wallets/src/components/WalletModal/index.tsx Outdated
@@ -0,0 +1,4 @@
import WalletModal from './WalletModal';
import './WalletModal.scss';
Copy link
Contributor

@hamza-deriv hamza-deriv Sep 21, 2023

Choose a reason for hiding this comment

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

@yashim-deriv
Do we need to add scss files inside the components instead of index.ts file.
Suggestion needed.

Copy link
Contributor

@thisyahlen-deriv thisyahlen-deriv Sep 21, 2023

Choose a reason for hiding this comment

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

Oh yeah export { default as }
@adrienne-deriv

Copy link
Contributor Author

@adrienne-deriv adrienne-deriv Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If there's only 1 component being exported in the folder, usually we can put the css import statement in the index.ts file right?

hamza-deriv
hamza-deriv reviewed Sep 21, 2023
View reviewed changes
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 21, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 24 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
13.2% 13.2% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@yashim-deriv yashim-deriv merged commit 14db8a3 into deriv-com:master Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yashim-deriv yashim-deriv yashim-deriv approved these changes

+2 more reviewers

@hamza-deriv hamza-deriv hamza-deriv left review comments

@thisyahlen-deriv thisyahlen-deriv thisyahlen-deriv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@adrienne-deriv @coveralls @yashim-deriv @thisyahlen-deriv @hamza-deriv