-
Notifications
You must be signed in to change notification settings - Fork 237
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
preliminary text display for formula using UI sheets #2470
Conversation
[sr-frontend] [Mon Sep 30 04:14:45 UTC 2024] - Deployed 8396898 to https://genshin-optimizer-prs.github.io/pr/2470/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Sep 30 04:30:56 UTC 2024] - Deployed 235c4af to https://genshin-optimizer-prs.github.io/pr/2470/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Sep 30 04:32:41 UTC 2024] - Deployed 4a87626 to https://genshin-optimizer-prs.github.io/pr/2470/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Sep 30 16:13:00 UTC 2024] - Deployed 16d8f7e to https://genshin-optimizer-prs.github.io/pr/2470/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Sep 30 17:19:59 UTC 2024] - Deployed 2e8da6c to https://genshin-optimizer-prs.github.io/pr/2470/sr-frontend (Takes 3-5 minutes after this completes to be available) [Tue Oct 1 02:00:53 UTC 2024] - Deleted 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.
Aside from moving TagMapSubset
to util functions, lgtm
import { charBaseUiSheet } from './sheets/CharBase' | ||
|
||
const tagValue: Array<{ tag: Tag; value: TagField }> = [] | ||
Object.values(uiSheets).forEach((sheet) => { |
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 be crawling another sheet to add to tagValue
? Can we somehow define both the sheets and tagValue
simultaneously? If not, this looks fine enough.
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 doubt the shape of sheets will change, so this crawling function will rarely need to be updated. I rather do this than to have to export a separate thing in sheets that will be mostly boilerplate per sheet.
const { q, elementalType } = tag | ||
if (q === 'heal' || q === 'heal_') return 'heal' | ||
if (elementalType) return elementalType | ||
// TODO: shield? |
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 check for top-level shield with q == "shield"
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.
Don't have a corresponding color for shield yet, will worry about it later.
* preliminary text display for formula using UI sheets * add base stats uisheet * fix heal * add more base stats, update sheet * createTagMap util
* preliminary text display for formula using UI sheets * add base stats uisheet * fix heal * add more base stats, update sheet * createTagMap util
Describe your changes
Take text attached to
TagField
in UI sheets to display for target selection in opt target selection.The main thing is the mapping of tag -> translations
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.