-
Notifications
You must be signed in to change notification settings - Fork 22
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(rakki): UI enhancement #1434
Conversation
New contract instantiated version Add missing UI elements Fix some styles
✅ Deploy Preview for gno-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for teritori-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
The bug in the Buy Tickets modal is out of scope of this PR or you want to spend more time in here to fix it ?
I don't really like this kind of big files with 600 lines, it's complicated to read, especially when it's the first time you read it ^^ But there is some components that are split inside so it's okay, and it's out of scope i understand that.
Else the design fit with the figma, but i don't have tori to test the feature on testnet, so i can't buy tickets to test the behaviour.
I splitted :) |
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.
Found the code better to read 👍 I don't know a lot this feature so i can just give my opinion on pure code/archi etc.
I tested to buy tickets, have i to test specific thing ? I tested to buy 1, and 20 tickets and it works
The feature's UX is pretty simple, there is nothing more you can test |
# Conflicts: # networks.json # packages/networks/teritori-testnet/index.ts # packages/screens/Rakki/RakkiScreen.tsx
import { RakkiClient } from "@/contracts-clients/rakki"; | ||
import { getKeplrSigningCosmWasmClient } from "@/networks/signer"; | ||
|
||
// TODO: replace all placeholders text with real values |
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 TODO is not addressed, we should address it or not remove it
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.
2f5bf64#diff-8e2c5b8031f032da60995a2ec1bed47f31b3db49f0ce3de39821c1d7f26382cfR21
What are these placeholders ? I thought it was good, my bad
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.
at the bottom, there is hardcoded $10k and stuff like that
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.
ad2ea23
to
70ff903
Compare
assets/logos/rakki-ticket.png
Outdated
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.
Should we reduce size of this image ?
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.
Yes, I used a PNG version because the SVG exported from Figma is glitched.
I asked help to Leonid
const { data: ticketsCount = null, ...other } = useQuery( | ||
["rakkiTicketsCountByUser", userId], | ||
async () => { | ||
if (!userId) { |
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.
I think we can use useQuery option: enabled: !!userId
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.
Yes!
a1ada4e
const [isButtonHovered, setButtonHovered] = useState(false); | ||
const [isModalVisible, setModalVisible] = useState(false); | ||
const remainingTickets = info.config.max_tickets - info.current_tickets_count; | ||
const [ticketAmount, setTicketAmount] = useState("1"); |
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.
Why this "amount" is in string format and we have to convert remainingTickets to string :o
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.
I forgot to remove this piece of code from here. See: #1434 (comment)
}> = ({ visible, setModalVisible, info, networkId }) => { | ||
const selectedWallet = useSelectedWallet(); | ||
const remainingTickets = info.config.max_tickets - info.current_tickets_count; | ||
const [ticketAmount, setTicketAmount] = useState("1"); |
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.
IMHO, if possible, we should use tickerAmount as number format , it's not a "big" number so we should process its value from the input to have the number format instead of string which is quite confusing
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.
Sorry for adding these changes..
I didn't modify that is this PR, just splitted code from RakkiScreen.tsx
.
This piece of code is the same as on origin/main
: https://github.com/TERITORI/teritori-dapp/blob/main/packages/screens/Rakki/RakkiScreen.tsx#L119
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 answer for this comment: #1434 (comment)
fontWeight: "600", | ||
}} | ||
> | ||
RAKKi |
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.
should it be RAKKI ? or Rakki ?
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.
It's RAKKi :)
Same as https://app.teritori.com/rakki
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.
LGTM
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.
LGTM
This is an enhancement of the UI of this: https://app.teritori.com/rakki
I splitted
RakkiScreen
in many components and made UI changes:RakkiScreen.tsx
(Just putted components in unitary files)Check the before: https://app.teritori.com/rakki
And the after: https://deploy-preview-1434--teritori-dapp.netlify.app/rakki
Figma: https://www.figma.com/design/WLEMAQXohIwBEPL5lsRrvK/Teritori---dApp?node-id=21858-204224&node-type=canvas&t=nEn47C5PtwMI9RWy-0
Highlights:
Connect Wallet button at the end of the flow (If user not connected)
This is now clickable and opens the BuyTicketsModal
In the modal, I put japanese text on the image
"Your tickets" is your tickets
"Deposit funds" if not enough funds