Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Scrollbar styles #2421

Merged
merged 11 commits into from
Feb 21, 2022
Merged

Scrollbar styles #2421

merged 11 commits into from
Feb 21, 2022

Conversation

alongoni
Copy link
Contributor

@alongoni alongoni commented Feb 14, 2022

Summary

Fixes #1145
Confirmation Modal:
image

Select a token modal:
Before/After

Styles on main scrollbar:
Before:
image
After:
image

To Test

@alongoni alongoni self-assigned this Feb 14, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2022

CLA Assistant Lite:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger the CLA Action by commenting recheckcla in this Pull Request

@alongoni alongoni added app:CowSwap CowSwap app Protofire Handled by Protofire development team labels Feb 14, 2022
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@alongoni alongoni added Low Severity indicator for defects. It won't cause any major break-down of the system Bug Something isn't working labels Feb 14, 2022
@alongoni alongoni marked this pull request as ready for review February 16, 2022 17:40
@elena-zh
Copy link

Hey @alongoni , great!

A couple of issues I have faced:

  1. I'm able to see 2 scroll bars when I open the menu in a tablet mode.
    double

In order to reproduce this issue, please expand Swap form as much as you can (specify a token pair with amounts, add recipient to the trade, etc). Then reduce the screen to the tablet mode and open the menu

  1. New Main scrollbar looks good in Chrome, but still it is not changed in FF. Will we be able to fix this as well?
    image

Thanks!

@alongoni
Copy link
Contributor Author

Hey @alongoni , great!

A couple of issues I have faced:

  1. I'm able to see 2 scroll bars when I open the menu in a tablet mode.
    double

In order to reproduce this issue, please expand Swap form as much as you can (specify a token pair with amounts, add recipient to the trade, etc). Then reduce the screen to the tablet mode and open the menu

  1. New Main scrollbar looks good in Chrome, but still it is not changed in FF. Will we be able to fix this as well?
    image

Thanks!

Hey @elena-zh

  1. I couldn't reproduce this issue with the double scroll (Chrome and FF). Can you give some context to reproduce it?
    Also I found another issue in medium devices. It seems that sometimes when we open the menu some options are cropped.
    https://www.loom.com/share/70ff6c55188a4cd699375f621e14849b

  2. Done! Now works fine on Firefox.

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Working great on FF to me!

Looping @biocom for another CSS pair of eyes

@elena-zh
Copy link

Hey @alongoni , I'm also no longer possible to reproduce the issue with a double scroll.

Scrollbar in FF looks' good to me now!

However, I have noticed another issue: when open a token list, and it is loading, you will see this (see the screenshot) for 2-3 sec
loading

Could we fix this?

Also, can we enhance this scrollbar?
this

@@ -30,7 +30,7 @@ export const AutoColumn = styled(AutoColumnUni)`

export const MobileWrapper = styled(AutoColumn)`
${({ theme }) => theme.mediaWidth.upToSmall`
isplay: none;
Copy link
Contributor Author

@alongoni alongoni Feb 18, 2022

Choose a reason for hiding this comment

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

Actually in mobile devices we can see the common tokens row.
image

What should we do with this?
If we fix this typo with display: none; , the common token list will disappear:
image

I believe we can keep it.
cc @elena-zh @alfetopito @biocom

Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave the token list as is. The horizontal scroller is expected and is only shown for resolutions smaller than desktop. On desktop they are just stacked. But on smaller devices, horizontal real estate is more precious. Further more, on actual touch devices (mobile/tablet) the scrollbar is not shown.

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

LGTM!

@alongoni alongoni added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Feb 21, 2022
@mergify mergify bot merged commit 6dc3d57 into develop Feb 21, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2022
@alongoni alongoni deleted the 1145-scrollbar-styles branch February 21, 2022 14:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:CowSwap CowSwap app Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds Bug Something isn't working Low Severity indicator for defects. It won't cause any major break-down of the system Protofire Handled by Protofire development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make scroll bar look nicer when it appears in the confirmation modal
5 participants