-
Notifications
You must be signed in to change notification settings - Fork 236
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
SRO Bonus stats UI #2482
SRO Bonus stats UI #2482
Conversation
[sr-frontend] [Fri Oct 4 03:53:20 UTC 2024] - Deployed f7ed084 to https://genshin-optimizer-prs.github.io/pr/2482/sr-frontend (Takes 3-5 minutes after this completes to be available) [Sat Oct 5 02:05:14 UTC 2024] - Deleted deployment [sr-frontend] [Mon Oct 7 15:56:40 UTC 2024] - Deployed a8aee38 to https://genshin-optimizer-prs.github.io/pr/2482/sr-frontend (Takes 3-5 minutes after this completes to be available) |
@@ -24,6 +25,7 @@ export interface Loadout { | |||
|
|||
buildTcIds: string[] | |||
optConfigId: string | |||
bonusStats: Array<{ tag: Tag; value: number }> |
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.
just curious, why are we using Array<> syntax over []?
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 { tag: Tag; value: number }[]
looks jank, imo
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 can use TagMapEntries<number>
setValue: (value: number) => void | ||
onDelete: () => void | ||
}) { | ||
// TODO: best way to infer percentage from tag? |
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 is the correct way
> | ||
<SqBadge sx={{ m: 'auto' }}>{tag.q}</SqBadge> | ||
{tag.q === 'dmg_' && ( | ||
<ElementTypeDropdown |
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.
technically this doesn't matter for HSR. I'm not aware of any character or mechanic that we would support in SRO that can do multi-element
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.
damageType1 and damageType2 would be better candidates, if we want to be technical about 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.
I think you misunderstand. this differentiates between dmg_
and ${element}_dmg_
, this is not a multi select.
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.
shouldn't really be in 'tabs', but i assume this is just temporary anyways
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 im waiting for design team to come up with some preliminary before going into tabs
Describe your changes
Add initial UI to add and edit SRO bonus stats, need additional work to link to calc.
Issue or discord link
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-ci
locally to validate format and lint.