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

Swap page layout #2488

Merged
merged 4 commits into from
Feb 25, 2022
Merged

Swap page layout #2488

merged 4 commits into from
Feb 25, 2022

Conversation

alongoni
Copy link
Contributor

@alongoni alongoni commented Feb 22, 2022

Summary

Fixes #1276 and #1550

The layout has changed a bit. Some small resolutions now have less padding on top but the scroll not appear.
Before (on production with scroll):
image
image

After (on this PR without scroll):
image
image

To Test

  1. <> Open the page Swap

@alongoni alongoni added Bug Something isn't working app:CowSwap CowSwap app Protofire Handled by Protofire development team labels Feb 22, 2022
@alongoni alongoni self-assigned this Feb 22, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 22, 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

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@alongoni alongoni marked this pull request as ready for review February 22, 2022 21:49
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!

@fairlighteth
Copy link
Contributor

Thanks for checking this. My comments:

  1. I'd add a bit more margin between header and the container.
  2. Please double check all the other (content) pages if this still looks right (incl. mobile).

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.

Looks good to me, just make sure Michel's comments are accounted for and I think it's good to go

@alongoni
Copy link
Contributor Author

alongoni commented Feb 23, 2022

Please @elena-zh can you test it again? Thanks!
cc @biocom

@elena-zh
Copy link

Changes LGTM.
The only difference I see in UI in comparison with Prod, is that there is a smaller margin between footer elements and the end of the content page in a tablet/mobile views
footer elements

But again, we have #1550 issue on the board that is related to footer elements displaying, so the issue might be fixed there.

@alfetopito , @biocom , WDYT?

@alongoni
Copy link
Contributor Author

hey @elena-zh Thanks. I can tackle #1550 issue in this PR.

@elena-zh
Copy link

@alongoni , looks much-much better.
The only issue I could mention is that the footer is displayed in a 'hovered' state when a screen resolution is less than 720 px. See the video: https://watch.screencastify.com/v/n5Gmhpbn6a4cS4X4tUgA

Can we fix it somehow?

@alongoni
Copy link
Contributor Author

@alongoni , looks much-much better. The only issue I could mention is that the footer is displayed in a 'hovered' state when a screen resolution is less than 720 px. See the video: https://watch.screencastify.com/v/n5Gmhpbn6a4cS4X4tUgA

Can we fix it somehow?

Yes, I saw it. But I think this behavior is ok since in mobile we don't have a :hover state. So in mobile the text has by default the max contrast between the text color and the background. Maybe we can leave it as it is.

@alongoni alongoni linked an issue Feb 24, 2022 that may be closed by this pull request
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.

Approved then!

@alongoni alongoni added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Feb 25, 2022
@mergify mergify bot merged commit 67d6294 into develop Feb 25, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Feb 25, 2022
@alongoni alongoni deleted the 1276-scrollbar-on-swap-page branch February 25, 2022 13:01
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 Protofire Handled by Protofire development team
Projects
None yet
5 participants