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

User collections create/edit route #425

Merged

Conversation

prabalsingh24
Copy link
Contributor

Problem

Added Collection-Create/Edit Route

@prabalsingh24 prabalsingh24 self-assigned this May 12, 2020
@prabalsingh24 prabalsingh24 changed the title User collections create route [WIP] User collections create route May 12, 2020
@coveralls
Copy link

coveralls commented May 12, 2020

Coverage Status

Coverage decreased (-0.9%) to 59.501% when pulling 21159eb on prabalsingh24:user-collections-createRoute into 5e8f534 on bookbrainz:UserCollection.

@prabalsingh24
Copy link
Contributor Author

prabalsingh24 commented May 12, 2020

This won't work rn. I made some changes in the bookshelf data and forgot to update it my project.
Edit : fixed it

src/client/components/forms/userCollection.js Show resolved Hide resolved
src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/server/helpers/collectionRouteUtils.js Outdated Show resolved Hide resolved
src/server/helpers/middleware.js Outdated Show resolved Hide resolved
@prabalsingh24
Copy link
Contributor Author

prabalsingh24 commented May 12, 2020

Thanks for a super quick review @MonkeyDo (I know you were busy w LB stuff :P )

@prabalsingh24
Copy link
Contributor Author

prabalsingh24 commented May 27, 2020

Screenshot_2020-05-28 BookBrainz – The Open Book Database

@prabalsingh24 prabalsingh24 changed the title [WIP] User collections create route User collections create/edit route May 31, 2020
Copy link
Member

@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.

Preliminary review, before trying to run the code.
Looking good so far !

src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/server/helpers/collectionRouteUtils.js Outdated Show resolved Hide resolved
src/server/helpers/collectionRouteUtils.js Outdated Show resolved Hide resolved
src/server/helpers/middleware.js Show resolved Hide resolved
src/server/routes/collection.js Outdated Show resolved Hide resolved
src/server/routes/collection.js Outdated Show resolved Hide resolved
src/server/routes/search.js Outdated Show resolved Hide resolved
Copy link
Member

@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.

Here's a review of the user interface, with a few improvements.

src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/client/components/forms/userCollection.js Show resolved Hide resolved
src/client/components/forms/userCollection.js Show resolved Hide resolved
src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/client/components/forms/userCollection.js Show resolved Hide resolved
src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
@prabalsingh24
Copy link
Contributor Author

Here's a review of the user interface, with a few improvements.

@MonkeyDo Thank you for the review. I will make these changes :)

Copy link
Member

@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.

A couple more comments after running into an issue locally :)

src/server/helpers/collectionRouteUtils.js Outdated Show resolved Hide resolved
src/client/components/forms/userCollection.js Outdated Show resolved Hide resolved
src/server/helpers/collectionRouteUtils.js Outdated Show resolved Hide resolved
src/server/helpers/collectionRouteUtils.js Show resolved Hide resolved
add fontawesome icons;
remove duplicate collaborators in cleanedCollaborator function
Copy link
Member

@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.

There's still a couple of unresolved items from previous reviews, but after that I think it'll be ready to merge 👍

src/client/components/forms/userCollection.js Show resolved Hide resolved
src/server/routes/search.js Outdated Show resolved Hide resolved
src/server/helpers/collectionRouteUtils.js Show resolved Hide resolved
src/client/components/forms/userCollection.js Show resolved Hide resolved
@prabalsingh24
Copy link
Contributor Author

prabalsingh24 commented Jun 9, 2020

I didn't add <hr/>. Is this alright?

Screenshot_2020-06-09 BookBrainz – The Open Book Database(1)

@prabalsingh24
Copy link
Contributor Author

I didn't add console.error(, Had to suppress eslint for that.

Copy link
Member

@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.

Great, ready to merge :)

@MonkeyDo MonkeyDo merged commit f15898a into metabrainz:UserCollection Jun 9, 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
Development

Successfully merging this pull request may close these issues.

3 participants