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

profile minors fix #1604

Merged
merged 9 commits into from
Oct 21, 2021
Merged

profile minors fix #1604

merged 9 commits into from
Oct 21, 2021

Conversation

maria-vslvn
Copy link
Contributor

@maria-vslvn maria-vslvn commented Oct 13, 2021 •
edited by alongoni
Loading

Summary

Fixes #1545

  1. Copy icon was given a left indent of 8px
  2. distance between 'Your referral url' and a referral link was increased
  3. Margins between icons and texts are increased

To Test

  1. go to the profile page

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

alongoni
alongoni suggested changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@alongoni alongoni left a comment

Choose a reason for hiding this comment

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

Looks great, Maria! However, I have a comment:

Reduce the Title <h3> margin bottom here:

cowswap/src/custom/pages/Profile/styled.tsx

Line 98 in 32653d2

margin: 0 0 34px 0;

From: margin: 0 0 34px 0; to margin: 0 0 16px 0;

@fairlighteth
Copy link
Contributor

A comment on this: It appears to me line-height is being used here, to achieve the effect of padding/margin (noteably top/bottom margin). Personally I think it's inappropriate to use line-height for that. When unexpected text wrapping occurs, you suddenly have wide gaps between text lines. I'd recommend using top/bottom padding for these scenarios instead.

@elena-zh
Copy link

Changes LGTM, however I have a question related to the margins inside of Trades/Referrals sections.
@maria-vslvn made them 20 px, however, in the mock-up I see 30 px bottom margin in the desktop view, and 14 px for the top one.
30-14

In the mobile view I see 14 px everywhere
14

@alongoni , what should we change it or we can leave it as it is (20 px for top/bottom/left/right)?

@alongoni
Copy link
Contributor

alongoni commented Oct 14, 2021 •
edited
Loading

Changes LGTM, however I have a question related to the margins inside of Trades/Referrals sections. @maria-vslvn made them 20 px, however, in the mock-up I see 30 px bottom margin in the desktop view, and 14 px for the top one. 30-14

In the mobile view I see 14 px everywhere 14

@alongoni , what should we change it or we can leave it as it is (20 px for top/bottom/left/right)?

Hey @elena-zh and @maria-vslvn, we can leave the padding as it is:

  • desktop: 20px
  • mobile: 14px

@maria-vslvn maria-vslvn requested a review from alongoni October 18, 2021 14:31
@alongoni alongoni added app:CowSwap CowSwap app Protofire Handled by Protofire development team labels Oct 18, 2021
alongoni
alongoni suggested changes Oct 18, 2021
View reviewed changes
@elena-zh
Copy link

It might be considered as a nitpick, but mobile paddings should be 14px ;)
image

@alongoni
Copy link
Contributor

alongoni commented Oct 20, 2021 •
edited
Loading

Looks great, Maria! However, I have a comment:

Reduce the Title <h3> margin bottom here:

cowswap/src/custom/pages/Profile/styled.tsx

Line 98 in 32653d2

margin: 0 0 34px 0;

From: margin: 0 0 34px 0; to margin: 0 0 16px 0;

Looks better! @maria-vslvn
I think the only thing missing is this adjustment in the Title margin.

@alongoni
Copy link
Contributor

alongoni commented Oct 20, 2021 •
edited
Loading

I found this issue with the logo. It's disappear on mobile:
https://pr1604--gpswapui.review.gnosisdev.com/#/profile
image (3)

Here works fine: https://pr1602--gpswapui.review.gnosisdev.com/#/profile
image (4)

cc @elena-zh

@elena-zh
Copy link

Yes, @alongoni , thanks! The cow icon is missing when screen width is less than 485 px.
@maria-vslvn , could you please fix it?
Please, use behavior as it is currently implemented in the Prod version: https://cowswap.exchange/#/swap

elena-zh
elena-zh approved these changes Oct 21, 2021
View reviewed changes
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 self-requested a review October 21, 2021 13:42
alongoni
alongoni approved these changes Oct 21, 2021
View reviewed changes
Copy link
Contributor

@alongoni alongoni left a comment

Choose a reason for hiding this comment

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

Looks great Maria! thanks.

Now the logo appears on mobile 👍
image

@alfetopito alfetopito changed the base branch from develop to release/1.4.0 October 21, 2021 18:10
@alfetopito alfetopito changed the base branch from release/1.4.0 to develop October 21, 2021 18:11
@alfetopito
Copy link
Contributor

FYI, I've changed the target branch from develop to release/1.4.0 but there were too many conflicts and would need a rebase.

I've changed it back and will cherry pick from develop to release/1.4.0 afterwards

@alfetopito alfetopito merged commit b29b3ba into develop Oct 21, 2021
@alfetopito alfetopito deleted the feature/1545-profile-ui-minors branch October 21, 2021 18:13
@alfetopito alfetopito restored the feature/1545-profile-ui-minors branch October 21, 2021 18:14
@alfetopito
Copy link
Contributor

I screwed up again 🤦

I'm sure sure I chose "squash and merge" this time, don't know what went wrong.

Will fix it...

alfetopito pushed a commit that referenced this pull request Oct 21, 2021
Profile minor fixes (PR #1604)
967ef4f 
Re-done the merging manually as I screw up and merged instead of
squash-merged

---

Squashed commit of the following:

commit e3de27a
Author: Maria Yablonskaya <[email protected]>
Date:   Thu Oct 21 12:17:38 2021 +0300

    fix header mob logo & minor fix

commit 5f4bd3d
Merge: ba2d58e a4a565a
Author: Maria Yablonskaya <[email protected]>
Date:   Thu Oct 21 12:04:46 2021 +0300

    Merge branch 'develop' of github.com:gnosis/cowswap into feature/1545-profile-ui-minors

commit ba2d58e
Author: Maria Yablonskaya <[email protected]>
Date:   Wed Oct 20 14:27:31 2021 +0300

    profile more minors fixed

commit 445a282
Merge: 574f7cf f790346
Author: Maria Yablonskaya <[email protected]>
Date:   Wed Oct 20 14:24:34 2021 +0300

    Merge branch 'develop' of github.com:gnosis/cowswap into feature/1545-profile-ui-minors

commit 574f7cf
Merge: 02e719d 32653d2
Author: Maria Yablonskaya <[email protected]>
Date:   Mon Oct 18 17:20:26 2021 +0300

    Merge branch 'feature/1545-profile-ui-minors' of github.com:gnosis/cowswap into feature/1545-profile-ui-minors

commit 02e719d
Author: Maria Yablonskaya <[email protected]>
Date:   Mon Oct 18 17:19:56 2021 +0300

    increase line-height

commit 47faa4a
Merge: 845d89a 2ccf897
Author: Maria Yablonskaya <[email protected]>
Date:   Mon Oct 18 17:11:58 2021 +0300

    Merge branch 'develop' of github.com:gnosis/cowswap into feature/1545-profile-ui-minors

commit 32653d2
Author: Lint Action <[email protected]>
Date:   Wed Oct 13 15:16:11 2021 +0000

    Fix code style issues with Prettier

commit 845d89a
Author: Maria Yablonskaya <[email protected]>
Date:   Wed Oct 13 18:06:02 2021 +0300

    profile minors fix
@alfetopito alfetopito deleted the feature/1545-profile-ui-minors branch October 21, 2021 18:20
@alfetopito
Copy link
Contributor

Fixed on 967ef4f

alfetopito pushed a commit that referenced this pull request Oct 21, 2021
Profile minor fixes (PR #1604)
570a406 
Re-done the merging manually as I screw up and merged instead of
squash-merged

---

Squashed commit of the following:

commit e3de27a
Author: Maria Yablonskaya <[email protected]>
Date:   Thu Oct 21 12:17:38 2021 +0300

    fix header mob logo & minor fix

commit 5f4bd3d
Merge: ba2d58e a4a565a
Author: Maria Yablonskaya <[email protected]>
Date:   Thu Oct 21 12:04:46 2021 +0300

    Merge branch 'develop' of github.com:gnosis/cowswap into feature/1545-profile-ui-minors

commit ba2d58e
Author: Maria Yablonskaya <[email protected]>
Date:   Wed Oct 20 14:27:31 2021 +0300

    profile more minors fixed

commit 445a282
Merge: 574f7cf f790346
Author: Maria Yablonskaya <[email protected]>
Date:   Wed Oct 20 14:24:34 2021 +0300

    Merge branch 'develop' of github.com:gnosis/cowswap into feature/1545-profile-ui-minors

commit 574f7cf
Merge: 02e719d 32653d2
Author: Maria Yablonskaya <[email protected]>
Date:   Mon Oct 18 17:20:26 2021 +0300

    Merge branch 'feature/1545-profile-ui-minors' of github.com:gnosis/cowswap into feature/1545-profile-ui-minors

commit 02e719d
Author: Maria Yablonskaya <[email protected]>
Date:   Mon Oct 18 17:19:56 2021 +0300

    increase line-height

commit 47faa4a
Merge: 845d89a 2ccf897
Author: Maria Yablonskaya <[email protected]>
Date:   Mon Oct 18 17:11:58 2021 +0300

    Merge branch 'develop' of github.com:gnosis/cowswap into feature/1545-profile-ui-minors

commit 32653d2
Author: Lint Action <[email protected]>
Date:   Wed Oct 13 15:16:11 2021 +0000

    Fix code style issues with Prettier

commit 845d89a
Author: Maria Yablonskaya <[email protected]>
Date:   Wed Oct 13 18:06:02 2021 +0300

    profile minors fix
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alfetopito alfetopito alfetopito approved these changes

@alongoni alongoni alongoni approved these changes

@elena-zh elena-zh elena-zh approved these changes

@ramirotw ramirotw Awaiting requested review from ramirotw

@fairlighteth fairlighteth Awaiting requested review from fairlighteth

Assignees
No one assigned
Labels
app:CowSwap CowSwap app Protofire Handled by Protofire development team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Profile page: UI issues with copy icon and margins between elements
6 participants
@maria-vslvn @fairlighteth @elena-zh @alongoni @alfetopito @lint-action