-
Notifications
You must be signed in to change notification settings - Fork 243
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: gift user modal #4091
feat: gift user modal #4091
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
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.
You are doing the extra disabled (isPlus badge later?)
Only one minor thing on the label should come from paddle I think.
<Typography | ||
bold | ||
className="rounded-10 bg-action-upvote-float px-2 py-1" | ||
type={TypographyType.Caption1} | ||
color={TypographyColor.StatusSuccess} | ||
> | ||
2 months free | ||
</Typography> |
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 should come from the data from Paddle no?
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.
Right. Although the one-time payment details will be adjusted from a different task, I will now make this label be coming from the object by paddle so it won't be forgotten.
Do you know which property this will be contained from Paddle?
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.
@sshanzel You can find a reference in PlusCard
and PlusInfo
components (look for "extraLabel"), maybe we could create an utility component for handling this kind of labels (not mandatory) 👀
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.
Yeah, I think that would be amazing. Let me check if I can easily do that here.
Am I able to search for myself? I think that I should not be able to pick me 👀 |
The search mutation was from mentioning a user, and from that mutation, yes, it will filter out the typing user. |
}); | ||
const isVisible = !!users?.length && !!query?.length; | ||
const onKeyDown = (e: React.KeyboardEvent) => { | ||
const movement = ['ArrowUp', 'ArrowDown', 'Enter']; |
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.
We could use KeyboardCommand
and ArrowKey
enums wdyt?
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.
Oh, right. Forgot they existed. Let me update 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.
@sshanzel ping
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.
Totally forgot about this 🤦
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.
@ilasw should be available now 🚀
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.
Not much to add other than what's already been said. Looks great to me!
We should focus on getting this one merged as a lot of pending PRs are waiting on it :D
}; | ||
|
||
interface GiftPlusModalProps extends ModalProps { | ||
preSelected?: UserShortProfile; |
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.
no need to capitalize the S
export function GiftPlusModal(props: ModalProps): ReactElement { | ||
return ( | ||
<PaymentContextProvider> | ||
<GiftPlusModalComponent {...props} />{' '} |
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 space probably not necessary here ? 🤔
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.
Yep. I think it was linting that did this. I'll remove. Thank you!
export function GiftPlusModalComponent({ | ||
preSelected, | ||
...props | ||
}: GiftPlusModalProps): ReactElement { |
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.
Maybe we could to separate select from general component for:
- keep general modal clean
- reuse select with it's logic in plus page
What do you think?
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.
Definitely! I think on the Plus page task, we can tackle extracting this so that it would be easier to see who needs what in terms of the places that would render them. Wdyt?
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! 🚢
return ( | ||
<Modal | ||
{...props} | ||
isOpen |
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.
is this prop needed?
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.
Ah, should be not since it is always going to be true. Let me clean it up.
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.
Code wise looks good :)
{discountPlan.extraLabel} | ||
</Typography> | ||
<PlusPlanExtraLabel | ||
color={PlusLabelColor.Help} |
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 guess ideally it would come from paddle but it's ok for now.
Changes
Preview:
Note that I just opened it automatically on load just to try.
Screen.Recording.2025-01-22.at.6.02.08.PM.mov
Events
Did you introduce any new tracking events?
Experiment
Did you introduce any new experiments?
Manual Testing
Caution
Please make sure existing components are not breaking/affected by this PR
Preview domain
https://mi-751.preview.app.daily.dev