-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: #163: pair selector #204
Conversation
# Conflicts: # package.json # pnpm-lock.yaml
# Conflicts: # src/pages/trade/ui/pair-selector.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High quality work! Design-wise, a few more comments. Will let you make the call on these:
- The
Clear
button should have the frosted-glass background. Think there is a css filter for this.
- After clicking
Clear
the text remains in the input field. - The loading skeleton is missing some detail:
- Missing the "No results" design (let's make sure we have the error state too):
- We seem to be missing the "confirm" state. Is it because you felt it unnecessary?
- Question: would it be possible to pre-fetch the state before the user clicks on the component? That way, there really isn't a "loading" state the majority of users encounter.
JSON.stringify( | ||
pairs.map(pair => ({ | ||
base: pair.base.toJson(), | ||
quote: pair.quote.toJson(), | ||
})), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: would it be easier if we just stored symbol pairs versus the whole metadata objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would delay showing data to user. Idk, for me it's more complicated to calculate the Metadata from symbol than storing it like this
src/features/star-pair/store.ts
Outdated
if (typeof window !== 'undefined') { | ||
this.setup(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible issue: see connectionStore.setup()
in the App
component. This was added because this pattern produced hydration errors in the connect store. This may need to go likewise in that useEffect (?).
if (typeof window !== 'undefined') { | ||
this.setup(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as the other
Addressed all PR feedback. The UI/UX is now the same as it is supposed to be in Figma. Fixed the mobile screens too |
Closes #163
Rewamped pair selector with:
pair-selector.mp4