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

impr: consider numbers in pb #4946

Merged
merged 14 commits into from
Feb 22, 2024
Merged

Conversation

notreallystatic
Copy link
Contributor

Fixes:
#4823

@monkeytypegeorge monkeytypegeorge added the backend Server stuff label Jan 19, 2024
@monkeytypegeorge
Copy link
Collaborator

Continuous integration check(s) failed. Please review the failing check's logs and make the necessary changes. https://github.com/monkeytypegame/monkeytype/actions/runs/7578828078

@Miodec
Copy link
Member

Miodec commented Jan 19, 2024

Updating the types is not enough. You also need to handle this new property correctly when saving pbs, and make sure to display it on the frontend.

@Miodec Miodec added the waiting for update Pull requests or issues that require changes/comments before continuing label Jan 19, 2024
@notreallystatic
Copy link
Contributor Author

@Miodec Yeah, the pbs will be saved. Also, in UI it will be shown as a new personal best cause we will set the flag as true in response. Are there any other places in the UI, where we are showing the score explicitly that I need to handle? What else I am missing?

@Miodec
Copy link
Member

Miodec commented Jan 22, 2024

@Miodec Yeah, the pbs will be saved. Also, in UI it will be shown as a new personal best cause we will set the flag as true in response. Are there any other places in the UI, where we are showing the score explicitly that I need to handle? What else I am missing?

pb-tables-popup.ts

image

Copy link
Contributor

This PR is stale. Please trigger a re-run of the PR check action.

@github-actions github-actions bot added Stale Has not been updated in a while and removed Stale Has not been updated in a while labels Jan 29, 2024
@monkeytypegeorge monkeytypegeorge added the frontend User interface or web stuff label Feb 7, 2024
@@ -636,6 +637,7 @@ export async function saveLocalPB<M extends MonkeyTypes.Mode>(
).forEach((pb) => {
if (
pb.punctuation === punctuation &&
!!pb.numbers === numbers &&
Copy link
Member

Choose a reason for hiding this comment

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

I dont think !! is necessary here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked that the older personal-bests in a given mode won't have the numbers field. That's why I added this. Btw, I am also adding support for pace-caret and tags, so still working on that. Could you tell if I am missing anything else as well?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, youre right. All though maybe i would prefer (pb.numbers ?? false) === number

Copy link
Contributor

This PR is stale. Please trigger a re-run of the PR check action.

@github-actions github-actions bot added the Stale Has not been updated in a while label Feb 20, 2024
@monkeytypegeorge monkeytypegeorge added docs Related to Markdown files and documentation assets Languages, themes, layouts, etc. labels Feb 22, 2024
@monkeytypegeorge monkeytypegeorge removed the docs Related to Markdown files and documentation label Feb 22, 2024
@Miodec Miodec merged commit 1429c2c into monkeytypegame:master Feb 22, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assets Languages, themes, layouts, etc. backend Server stuff frontend User interface or web stuff Stale Has not been updated in a while waiting for update Pull requests or issues that require changes/comments before continuing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants