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

[Profile] Add explorer orders link to Profile page and app menu #1935

Merged
merged 9 commits into from
Dec 15, 2021

Conversation

matextrem
Copy link
Contributor

Summary

Fixes #1883

Added a link to Orders explorer from Profile page and App menu

Screen Shot 2021-11-28 at 21 36 49

To Test

  1. Open profile page
  • Connect an account
  • You'll see a View all orders link
  1. Open app menu clicking on the three dots in the upper right corner
  • Connect an account
  • You'll see a View all orders link

@matextrem matextrem added the Protofire Handled by Protofire development team label Nov 29, 2021
@matextrem matextrem self-assigned this Nov 29, 2021
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

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.

Hey @matextrem , good job! Now a user may see his/her orders from the menu/profile page.
However, some issues:

  1. I'd prefer hiding this empty section from the Profile page when a wallet is not connected/connected to an unsupported network
    hide when not connected
  2. Then, it would be good not to show it to a new user when he/she has not placed any order yet. It would be good to start displaying it when at least a user has 1 order in user's order lint in the Explorer
  3. 'View all orders' link should navigate a user not only to the https://gnosis-protocol.io/address/... , it should take into account the environment a user is working in:
  • cowswap.dev.gnosisdev.com --> protocol-explorer.dev.gnosisdev.com/
  • cowswap.staging.gnosisdev.com --> protocol-explorer.staging.gnosisdev.com
  • barn.cowswap.exchange --> /barn.gnosis-protocol.io
  • cowswap.exchange --> gnosis-protocol.io
  1. When connected to xDAI network. View all orders link navigates to the etherscan.io
    https://watch.screencastify.com/v/HOHCEiJ5HThSvzeW1J0p

Also, I have some doubts related to the UI. It seems to me that the section for the link is too big.. Could we make it a bit narrower?
image

Then, it might be goo to show 'View all orders' link at the top or at the bottom of the list to make it a bit more visible. @alfetopito , @alongoni , @biocom , WDYT?

@alongoni
Copy link
Contributor

Maybe we can show the "View all orders" link here:
image

@alfetopito
Copy link
Contributor

Great summary @elena-zh. Mati, whatever she said hehe

For 3, there's is a function for that

function _getExplorerBaseUrl(chainId: ChainId): string {

For 4, one could argue that we should always show mainnet orders since the affiliate program only counts them, but I agree with Elena and the expectation would be to link to the correct network, even if there's no tracking of those orders.

The same function above also takes care of that.

And I also like @alongoni 's suggestion. The only concern is how the top of the modal will look like on mobile.
Otherwise, I like the idea and would prefer it there instead of a whole row inside the modal.

@elena-zh
Copy link

elena-zh commented Dec 1, 2021

Hey @matextrem , great job!
Some cases:

  1. I would vote for moving 'View all orders' menu item to the bottom to make it more visible
    image

  2. 'View all orders' menu item is still displayed when it is not displayed in a Profile page to a user with no orders placed:
    remove from the men  as well

  3. There is no 'View all orders' link in the profile page when 'Last updated' date is not empty
    no link when last updated

  4. Also, different fonts and text colors for link and the date label look strange a bit.
    image

Could we display the link under the page header?
image
And in different lines in a mobile view
image

@alongoni , WDYT?

@alongoni
Copy link
Contributor

alongoni commented Dec 1, 2021

hey @elena-zh I was thinking to move the "Last updated" indicator next to the "Profile overview" title. In this way the "View all orders" link is more visible there. Do you think it could work better?
image
cc @alfetopito

@elena-zh
Copy link

elena-zh commented Dec 2, 2021

Hey @matextrem ,

  1. still I do not see 'View all orders' link in Profile page for accounts with no orders in Mainnet, but with orders in xDAI/Rinkeby. Btw, 'View all orders' link in the menu is displayed for these cases:
    no for xdai

Account details: https://protocol-explorer.dev.gnosisdev.com/xdai/address/0xFF714b8b0e2700303eC912BD40496C3997ceEa2b

  1. Also, it would be nice to remove a margin before Last updated field in a mobile view to make all lines center-aligned
    remove
    here the result of removing:
    result after removing

  2. It would be great if all fields in the lines have the same font-size:

  • mobile:
    make the same
  • desktop:

make the same desk

@alongoni , I still vote for moving these fields under the page name. But it is my personal point of view, and might be skipped if everyone else thinks differently.
vote

@matextrem
Copy link
Contributor Author

matextrem commented Dec 2, 2021

@elena-zh regarding 1, the thing is we are using /profile request info for showing/not showing orders link and using /trades request info for showing/not showing orders link on the app menu. It will be fixed once this PR is merged

@elena-zh
Copy link

elena-zh commented Dec 2, 2021

@matextrem , #1900 PR takes into account trades in the Mainnet only.
But 'view all orders' link should appear for all networks: XDAI, Rinkeby, if there are any orders there.
Another question is: why does 'view all orders' link works as expected in the menu, but does not in the profile page?

@matextrem
Copy link
Contributor Author

@elena-zh I'm using /profile request info for profile page and /trades request info for the app menu bar. So you are saying we are using the info given by /trades only ?

@elena-zh
Copy link

elena-zh commented Dec 2, 2021

@matextrem , seems that this one
/api/v1/account/ < account > /orders/?limit=100&offset=0

At least, it works from here
image

@alfetopito , am I correct?

@alfetopito
Copy link
Contributor

alfetopito commented Dec 2, 2021

Let's see, trying to address all comments in one place

  1. To check whether to display the View all orders link

Here we are looking at orders rather than trades, so use the function getOrders with limit=1, similar to how hasTrades function does it

  1. Where should View all orders point to

To Explorer's /address/<address> endpoint.
Simply use this function

export function getExplorerAddressLink(chainId: ChainId, address: string): string {

  1. View all orders placement on mobile

Looking great! Besides Elena's comments I have nothing to add

  1. View all orders placement on desktop

I'm not too keen on swapping the position with last updated as suggested.
Maybe Elena's idea of putting it on another row addresses it better?

I'd like to invoke @biocom 's Design powers to give his input and help us settle the matter.

@anxolin
Copy link
Contributor

anxolin commented Dec 2, 2021

Great to see we are adding the links

I could see the two things in the right corner, view orders, last update (below), from the ideas is the one that seems better.

Anyways, I wanted just to mention that we have a menu problem 😅
This need a full redesign, not everything there is equally important, and it's hard to use that menu.

Maybe not part of this PR, but I wouldn't add a new link unless we redesign it to acomodate better the content.

image

@anxolin
Copy link
Contributor

anxolin commented Dec 2, 2021

I will expand on my last comment regarding the menu.

I see that Discord, Twitter, Github could be just some nice icons that are located at the top or bottom of the menu
The links could be in sectors, where the About, Faq, Docs would be the highlight. Maybe even the stats, not sure.

Then the games could be in another section and presented differently.

@elena-zh elena-zh mentioned this pull request Dec 3, 2021
@elena-zh
Copy link

elena-zh commented Dec 3, 2021

@anxolin , I opened a new issue for the menu reorganizing #1956

@elena-zh
Copy link

elena-zh commented Dec 3, 2021

Works nicely in terms of functionality.
In terms of the menu, I have created a separate issue for this (mentioned above).
Still we need to find the final decision in terms of the link displaying on the form.

@fairlighteth
Copy link
Contributor

I think it makes sense to put the 'view all orders' in the right corner:
Screen Shot 2021-12-03 at 17 48 42

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.

Changes LGTM!

@ramirotw ramirotw requested a review from anxolin December 9, 2021 13:20
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.

👍

@matextrem matextrem merged commit 9ff8011 into develop Dec 15, 2021
@matextrem matextrem deleted the 1883/orders-link branch December 15, 2021 11:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Protofire Handled by Protofire development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Profile] Add a link to order list in Explorer
6 participants