Skip to content
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.

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

Add PairInfo tab #544

Merged
merged 36 commits into from
Aug 11, 2024
Merged

Add PairInfo tab #544

merged 36 commits into from
Aug 11, 2024

Conversation

saidam90
Copy link
Contributor

@saidam90 saidam90 commented Aug 6, 2024

solves #432

Added a tab for PairInfo.

@saidam90 saidam90 requested a review from a team as a code owner August 6, 2024 15:54
Copy link

cloudflare-workers-and-pages bot commented Aug 6, 2024

Deploying dexter with  Cloudflare Pages  Cloudflare Pages

Latest commit: 62bbf24
Status: ✅  Deploy successful!
Preview URL: https://3c5f3d3e.dexter-1we.pages.dev
Branch Preview URL: https://show-resource-addresses.dexter-1we.pages.dev

View logs

Copy link
Contributor

@dcts dcts 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 overall!

Some improvement proposals:

  1. Can you hide the candle timeselection whenever the pair info is selected?
Bildschirmfoto 2024-08-06 um 18 46 33
  1. Is it possible to always keep the tabs next to each other, and instead wrap the candle timeframe selection?
Bildschirmfoto 2024-08-06 um 18 52 03

Copy link
Contributor

@dcts dcts left a comment

Choose a reason for hiding this comment

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

Some more improvements:

  1. The two coin boxes should both take up exactly 50% of the full space, to align with the orderrInput and orderBook components. Currently left box it too small for viewport width of 740px, and for 840px viewportwidth the right box is too small. I think you can just add a "flex-grow" on both elements, but probably will need to fix also oversizing in case the content is too big.
Bildschirmfoto 2024-08-07 um 00 32 31
  1. I think to be consistent, all those elements marked red should have the same bottom margin. I suggest mb-1 for all those elements (maybe you'll need to use !mb-1 for <p> elements.
Bildschirmfoto 2024-08-07 um 00 42 44
  1. Please use the shortenString function to display a shorter version of the addresses, I suggest to use 20 for showEnd and to set showStart in a way that will show "resource..." or "component...", so either 8 or 9. This is a pattern shared among all crypto space, as people usually check the last few characters of an address.

  2. Can you add px-2 to both nav elements (Price chart + Pair info) to space out the tabs a bit?

Bildschirmfoto 2024-08-07 um 00 51 34
  1. Not neccesarily needed, but when copying it would be awesome if the user would get a feedback, something similar to this (found here)
Bildschirmfoto 2024-08-07 um 00 45 19

@dcts dcts self-assigned this Aug 7, 2024
@dcts dcts changed the title Show resource addresses Add PairInfo tab Aug 7, 2024
Copy link
Contributor

@dcts dcts left a comment

Choose a reason for hiding this comment

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

Amazing! Great work with the refactor and all the changes made!

package.json Show resolved Hide resolved
src/app/components/CopyIcon.tsx Outdated Show resolved Hide resolved
src/app/components/CopyIcon.tsx Outdated Show resolved Hide resolved
src/app/components/CopyIcon.tsx Outdated Show resolved Hide resolved
src/app/components/PriceChart.tsx Outdated Show resolved Hide resolved
src/app/state/locales/pt/trade.json Outdated Show resolved Hide resolved
src/app/state/priceChartSlice.ts Outdated Show resolved Hide resolved
src/app/components/PriceChart.tsx Outdated Show resolved Hide resolved
src/app/components/PriceChart.tsx Outdated Show resolved Hide resolved
src/app/components/PriceChart.tsx Outdated Show resolved Hide resolved
src/app/components/PriceChart.tsx Outdated Show resolved Hide resolved
src/app/components/TextToCopy.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@dcts dcts left a comment

Choose a reason for hiding this comment

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

🎉

@dcts dcts merged commit ee03961 into main Aug 11, 2024
2 checks passed
@dcts dcts deleted the show-resource-addresses branch August 11, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants