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

Added No. of Entities in Collection Page #742

Merged
merged 4 commits into from
Mar 1, 2022
Merged

Conversation

vishal5251
Copy link
Contributor

Problem

Display number of items in a collection on that collection's page (#BB-638)

Solution

I've added "No. of Entity" section in the Collection Page

Areas of Impact

/home/[UserName]/bookbrainz-site/src/client/components/pages/collection.js (line : 77-80)

@welcome
Copy link

welcome bot commented Dec 16, 2021

Thanks for opening your first pull request for BookBrainz, and welcome to our community! 🎉

If you haven't yet, please check out our contributing guidelines.

Someone will be reviewing your PR soon; just hang in there !
In the meantime, if you're wondering what to do next, you can have a look at our issue tracker

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 ! That's a pretty straightforward change but it's very much missing !

src/client/components/pages/collection.js Outdated Show resolved Hide resolved
@vishal5251
Copy link
Contributor Author

what Is the missing part here can you brief this then I'll will fix it.

@MonkeyDo
Copy link
Contributor

what Is the missing part here can you brief this then I'll will fix it.

Hi @vishal5251 !
Sorry I wasn't very clear !
What I meant is that showing the number of entities on the collection page is really missing, and your PR is very appreciated :)

I did make a small wording suggestion though, have a look at the PR review: #742 (comment)

Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
@MonkeyDo
Copy link
Contributor

That was quick ! Thanks a lot for your contribution and your reactivity !
I'll make sure tests pass and then merge.

@vishal5251
Copy link
Contributor Author

How could I be able to make its all checks passed. Any idea!

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.

Ah, I notice there is an issue with the tests, which is linked to the addition of a bookbrainz-site submodule in your repository fork.
Here's what I see in the changed files:
image

Notice the top part "Submodule bookbrainz-site added".
That shouldn't be there; not exactly sure how that happened but you'll need to fix it before I can merge.
If you struggle finding how to fix it, and considering this PR has minimal changes, you could also redo the changes in a new branch and open another PR.

Let me know if I can help !

@MonkeyDo MonkeyDo merged commit 118d01f into metabrainz:master Mar 1, 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