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: Adding Entity from Collection Page #487

Conversation

prabalsingh24
Copy link
Contributor

Problem

Adding Entity from Collection Page

Screenshot_2020-08-10 BookBrainz – The Open Book Database(2)

@coveralls
Copy link

coveralls commented Aug 10, 2020

Coverage Status

Coverage decreased (-0.4%) to 57.707% when pulling 3b0816c on prabalsingh24:addEntityToCollection-CollectionPage into d1fd35b on bookbrainz:UserCollection.

@prabalsingh24
Copy link
Contributor Author

prabalsingh24 commented Aug 10, 2020

I am not able to solve these two bugs. Can you look at this please?

  1. there is a horizontal line below select edition. See the image above. Idk why it's showing up
  2. the error message is showing up like this. It's covering up the selection field.
    Screenshot_2020-08-10 BookBrainz – The Open Book Database(3)

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.

Looking nice !
This is going to be a very useful feature!

@prabalsingh24
Copy link
Contributor Author

What do you think of refreshDone function? I used it in remove entities feature as well. Earlier I was refreshing the whole page.

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 do like the last changes; having the success message show on the main page and autorefreshing the results will look really nice and smooth for the user.

I hadn't thought about the complication of having the pager component trigger the search, but I think with the suggestion below of using a reference it should be pretty seamless.
I think your solution definitely had merit, so well done for finding that :)

src/client/components/pages/parts/pager.js Outdated Show resolved Hide resolved
@prabalsingh24
Copy link
Contributor Author

I forgot about ref. That's why i was using refreshTable refreshDone :P

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'm super happy with how this is turning out! Really smooth!

One final tiny details before merging:

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

Great stuff!
This is going to make it much easier for users to create collections!

@MonkeyDo MonkeyDo merged commit 3e97733 into metabrainz:UserCollection Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants