-
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] Add game title to QuestDetailsWrapper component #52
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Good stuff! 🔥
got some conflicts to resolve on this one @biliesilva |
…into feat]-Add-game-title-to-QuestDetailsWrapper
@@ -152,8 +158,8 @@ export function QuestDetailsWrapper(props: QuestDetailsWrapperProps) { | |||
'Connect Steam account' | |||
), | |||
questType: { | |||
REPUTATION: t('quest.type.reputation', 'Reputation'), |
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 need to keep the reputation translation string
REPUTATION: t('quest.type.reputation', 'Reputation'), | ||
PLAYSTREAK: t('quest.type.playstreak', 'Play Streak') | ||
PLAYSTREAK: t('quest.type.playstreak', 'Play Streak'), | ||
GAME: gameName |
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.
game isn't a quest type. did you mean to add this prop somewhere else?
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.
These are two different badges, one is the type of quest, and the other is the game name.
@@ -68,6 +72,8 @@ export function QuestDetailsWrapper(props: QuestDetailsWrapperProps) { | |||
|
|||
const questResult = useGetQuest(selectedQuestId, getQuest) | |||
const questMeta = questResult.data?.data | |||
const projectId = questMeta?.project_id | |||
const gameName = projectId && listings?.[projectId]?.project_meta?.name |
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.
could we just pass in listings?.[projectId]?.project_meta?.name
as game name as a prop to this component instead of passing the whole listings object?
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 is a lot of data to pass around if we have to pass the listings object here too. This will probably be hard to adopt in store and client since they can use different listings interfaces
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.
So, this was something I was talking with @eliobricenov about, I will ping your conversations about it.
Description
Added the game listings directly to the badge in the component, next step is to update the client for this lib.
Storybook
Design Link