Skip to content
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

Tallies added for teams and loadouts #2070

Merged
merged 5 commits into from
May 6, 2024
Merged

Conversation

maoadrian12
Copy link
Contributor

Describe your changes

Added a tally when looking at databases for the number of teams and loadouts.

Issue or discord link

#2040

Testing/validation

BEFORE
image
AFTER
image

Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)

  • [ x ] I have commented my code in hard-to understand areas.
  • [ ] I have made corresponding changes to README or wiki.
  • [ ] For front-end changes, I have updated the corresponding English translations.
  • [ x ] I have run yarn run mini-ci locally to validate format and lint.
  • [ x ] If I have added a new library or app, I have updated the deployment scripts to ignore changes as needed

@frzyc frzyc marked this pull request as ready for review May 5, 2024 02:47
@frzyc frzyc marked this pull request as draft May 5, 2024 02:48
Copy link
Contributor

github-actions bot commented May 5, 2024

[frontend] [Sun May 5 02:51:20 UTC 2024] - Deployed 22c30a9 to https://genshin-optimizer-prs.github.io/pr/2070/frontend (Takes 3-5 minutes after this completes to be available)

[frontend] [Sun May 5 16:27:34 UTC 2024] - Deployed 1beb311 to https://genshin-optimizer-prs.github.io/pr/2070/frontend (Takes 3-5 minutes after this completes to be available)

[frontend] [Mon May 6 16:44:23 UTC 2024] - Deployed 004ed14 to https://genshin-optimizer-prs.github.io/pr/2070/frontend (Takes 3-5 minutes after this completes to be available)

[Mon May 6 16:52:45 UTC 2024] - Deleted deployment

Copy link
Owner

@frzyc frzyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Is it possible to move the last edit time here? Will balance out the whitespace.
Also, update translations in https://github.com/frzyc/genshin-optimizer/blob/master/libs/gi/localization/assets/locales/en/settings.json

@maoadrian12 maoadrian12 marked this pull request as ready for review May 5, 2024 16:08
@maoadrian12
Copy link
Contributor Author

Looks like this now:
image
Not sure if this is intended behavior or not, but when the loadouts are changed/updated, it doesn't change the lastEdit time listed under the database.

@frzyc
Copy link
Owner

frzyc commented May 5, 2024

Looks like this now: image Not sure if this is intended behavior or not, but when the loadouts are changed/updated, it doesn't change the lastEdit time listed under the database.

Looks much better now. That is a good catch on the lastEdit issue: #2072

Copy link
Owner

@frzyc frzyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to pass format CI stage:
assuming you are running a fork with upstream set to the main repo, you can run nx format write --base upstream/master. Next time, it might be better to create a feature branch in your fork to avoid this issue.

@@ -63,7 +63,17 @@ function DataCard({ index, readOnly }: { index: number; readOnly: boolean }) {
const numChar = database.chars.keys.length
const numArt = database.arts.values.length
const numWeapon = database.weapons.values.length
const hasData = Boolean(numChar || numArt || numWeapon)
const numTeams = database.teams.values.length
let numLoadouts = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number of loadouts should just be database.teamChars.values.length.

Can you also include the number of builds?
database.builds.values.length

@maoadrian12
Copy link
Contributor Author

Need to pass format CI stage: assuming you are running a fork with upstream set to the main repo, you can run nx format write --base upstream/master. Next time, it might be better to create a feature branch in your fork to avoid this issue.

How should I know if the formatting is correct, when I run the command(yarn run nx format:check) I get a lot of CRLF warnings, is that fine?
Besides that, here's what the screen looks like now:
image

@frzyc
Copy link
Owner

frzyc commented May 5, 2024

I get a lot of CRLF warnings, is that fine?

I think you need to change your autocrlf settings in git:

git config core.autocrlf false

@maoadrian12
Copy link
Contributor Author

When I do yarn run mini-ci, I only fail the gi-dm-localization test because the file at libs/gi/dm/GenshinData/TextMap/TextMapCHS.json isn't found, is this fine?

@frzyc
Copy link
Owner

frzyc commented May 5, 2024

That missing file is likely due to the datamine submodule not being loaded, you can run reload-dm for that. For the format issue, you can follow the instructions here #2070 (review). (mini-ci is suppose to run format, but it won't work for you here because you are working off your fork's master)

@maoadrian12
Copy link
Contributor Author

Did I break something? The checks have been going on for a while

@frzyc
Copy link
Owner

frzyc commented May 6, 2024

Did I break something? The checks have been going on for a while

Nope, because you are a first contributor, the pipeline needs approval before running. I've started it.

@frzyc
Copy link
Owner

frzyc commented May 6, 2024

LGTM, thanks for your work!.

@frzyc frzyc merged commit 548b32d into frzyc:master May 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants