Skip to content

shontzu/WEBREL-1584/Extra-spaces-in-jurisdiction-modal #11060

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
Closed
wants to merge 4 commits into from
Closed

shontzu/WEBREL-1584/Extra-spaces-in-jurisdiction-modal #11060

wants to merge 4 commits into from

Conversation

shontzu-deriv
Copy link
Contributor

@shontzu-deriv shontzu-deriv commented Oct 31, 2023 •
edited
Loading

Changes:

blank whitespaces

Screenshots:

Before:

image

After:

Screen.Recording.2023-10-31.at.3.40.39.PM.mov

@vercel
Copy link

vercel bot commented Oct 31, 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 Oct 31, 2023 11:16am

@shontzu-deriv shontzu-deriv changed the title style: height correction for derived/financial/all shontzu-deriv:shontzu/WEBREL-1584/Extra-spaces-in-jurisdiction-modal Oct 31, 2023
@shontzu-deriv shontzu-deriv changed the title shontzu-deriv:shontzu/WEBREL-1584/Extra-spaces-in-jurisdiction-modal shontzu/WEBREL-1584/Extra-spaces-in-jurisdiction-modal Oct 31, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 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/11060](https://github.com/binary-com/deriv-app/pull/11060)
- **URLs**:
    - **w/ App ID + Server**: https://deriv-app-git-fork-shontzu-deriv-shontzu-webrel-1584extr-da02a5.binary.sx?qa_server=red.derivws.com&app_id=24059
    - **Original**: https://deriv-app-git-fork-shontzu-deriv-shontzu-webrel-1584extr-da02a5.binary.sx
- **App ID**: `24059`

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2023 •
edited
Loading

🚨 Lighthouse report for the changes in this PR:

Category Score
🔺 Performance 15
🟧 Accessibility 75
🟢 Best practices 92
🟧 SEO 85
🟧 PWA 80

Lighthouse ran with https://deriv-app-git-fork-shontzu-deriv-shontzu-webrel-1584extr-da02a5.binary.sx/

hamza-deriv
hamza-deriv approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@hamza-deriv hamza-deriv left a comment

Choose a reason for hiding this comment

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

Lgtm

mahdiyeh-deriv
mahdiyeh-deriv reviewed Oct 31, 2023
View reviewed changes
packages/appstore/src/components/cfds-listing/cfds-listing.scss Outdated
&__wrapper {
@include desktop {
height: 76rem;
height: 69rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
height: 69rem;
max-height: 76rem;

mahdiyeh-deriv
mahdiyeh-deriv reviewed Oct 31, 2023
View reviewed changes
packages/appstore/src/components/cfds-listing/cfds-listing.scss Outdated
height: 69rem;
perspective: 100rem;
overflow: scroll;
&:has(.jurisdiction-modal__scrollable-content .cfd-jurisdiction-card--financial__wrapper) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not good idea to add conditions to the css

maryia-deriv
maryia-deriv suggested changes Oct 31, 2023
View reviewed changes
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
2.2% 2.2% Duplication

maryia-deriv
maryia-deriv suggested changes Oct 31, 2023
View reviewed changes
packages/appstore/src/components/cfds-listing/cfds-listing.scss
&__footnotes-container {
@include desktop {
gap: 1rem;
min-height: 7.6rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

still reproducible, i think .jurisdiction-modal__wrapper should have height: 69rem;. please check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will cause overflow and scrolling on financial because of having longer content.... it is possible to use conditional heights, i.e. 69rem for derived and swap free, 76rem for financial but it might become a best practice issue as mentioned in above review

cc: @mahdiyeh-deriv

@shontzu-deriv shontzu-deriv deleted the shontzu/WEBREL-1584/Extra-spaces-in-derived-and-swapfree-jurisdiction-modal branch November 3, 2023 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vinu-deriv vinu-deriv vinu-deriv approved these changes

@matin-deriv matin-deriv Awaiting requested review from matin-deriv matin-deriv is a code owner

@nijil-deriv nijil-deriv Awaiting requested review from nijil-deriv nijil-deriv is a code owner

+4 more reviewers

@mahdiyeh-deriv mahdiyeh-deriv mahdiyeh-deriv left review comments

@maryia-deriv maryia-deriv maryia-deriv requested changes

@hamza-deriv hamza-deriv hamza-deriv approved these changes

@arshad-rao-deriv arshad-rao-deriv arshad-rao-deriv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AppStore

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@shontzu-deriv @mahdiyeh-deriv @vinu-deriv @maryia-deriv @hamza-deriv @arshad-rao-deriv