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

Feat(Achievement): Show achievement progress #844

Merged
merged 3 commits into from Jun 21, 2022

Conversation

tr1ten
Copy link
Collaborator

@tr1ten tr1ten commented May 23, 2022

Problem

BB-505: Show achievement progress

Areas of Impact

achievement, editor

Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

I get an error when I run this PR locally and try to open my achievements page:

error: select "bookbrainz"."_editor_entity_visits".* from "bookbrainz"."_editor_entity_visits" where "editor_id" = $1 - relation "bookbrainz._editor_entity_visits" does not exist

The table does exist in production, but not in the database dumps I guess, which includes the one used on test.bookbrainz.org
How did you test it locally?

@tr1ten
Copy link
Collaborator Author

tr1ten commented Jun 10, 2022

The table does exist in production, but not in the database dumps

I thought so this would be the case here thus i created one for testing purposes

Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Thanks a ton for fixing some achievements and adding the progress, I think it will satisfy some users.
I'm not too fussed about the achievement system myself, but it's definitely an improvement with this PR !

src/server/helpers/achievement.js Show resolved Hide resolved
return getEditsInDays(orm, editorId, 6);
}
if (achievementId === 12) {
return getEditsInDays(orm, editorId, 29);
Copy link
Contributor

Choose a reason for hiding this comment

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

Testing locally, I can see this achievement doesn't work as expected.
It counts days where I made a revision, but not consecutive days.
So for example if I made a revision on the 1st, 2nd and 3rd of the month, then nothing for a few days, then another revision on the 6th, the progress should be 0.
Instead, currently it is going to return 4.

I suspect the same goes for "Fun Runner" (achievement # 10).

This doesn't need to be fixed in this PR, since it does not result from these changes, I will create a ticket for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

src/server/routes/editor.js Show resolved Hide resolved
{!this.state.unlocked &&
<Col>
<div className="h3">
{this.state.counter}/{maxAchievementProgress[this.state.achievement.id] ?? 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a progress bar in here?
We can show the percentage as a label in the progress bar, and center all of it.:
https://react-bootstrap-v4.netlify.app/components/progress/#progress-label

Something along those lines; what do you think?
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Progress bar does seem like a good idea, how about a circular one (eg), placing text at its center?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's an interesting idea, and would look quite good.
However I would suggest doing so in a separate PR later on, as it seems to me the whole page needs many visual improvements. I don't want to sidetrack you from your GSoC project.
It'll be a good improvement to go with the rest, and in the meantime we avoid adding another dependency when we already have a progress component in react-bootstrap.

@MonkeyDo MonkeyDo merged commit 3335e43 into metabrainz:master Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants