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

Card component #34

Merged
5 commits merged into from
Oct 20, 2020
Merged

Card component #34

5 commits merged into from
Oct 20, 2020

Conversation

ghost
Copy link

@ghost ghost commented Oct 15, 2020

The Card component doesn't appear on the page because the volumes array in CardsList is empty. To test Card, populate the volumes array with props for a Card, eg

export default function CardList() {
  const volumes = [{ title: "Foo", authors: ["Bar", "Baz"] }];
  const card = (volume) => <Card {...volume} />;
  return <div>{volumes.map(card)}</div>;
}

@ghost ghost requested a review from snrelghgub October 15, 2020 19:03
@ghost ghost linked an issue Oct 15, 2020 that may be closed by this pull request
3 tasks
@snrelghgub
Copy link
Contributor

hello @willnwhite Your PR is linked to the issue "Display Search Results" which requires the books matching the search criteria display in an infinite scroll, but your PR does not resolve the issue. Please review your code or requirements or let me know or anyone else on our team if you need any assistance with resolving your assigned issue 👍

@ghost
Copy link
Author

ghost commented Oct 19, 2020

Still no infinite scroll but user can see search results on page.

@ghost
Copy link
Author

ghost commented Oct 19, 2020

Infinite scroll moved to #39 so this branch can be merged now.

Copy link
Contributor

@snrelghgub snrelghgub left a comment

Choose a reason for hiding this comment

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

I reviewed your PR once again & tested with different inputs & the correct results displays, the requirement is met & that is the most important. However, the 'more' button provides a UX flaw. The problem with it is that after the User clicks on the 'more' button the next batch of results is displayed upwards meaning that the User has to scroll UP to view the next batch of results from bottom to a new ambiguous starting point. If the User wants to view 'more' again, the User will have to scroll back down to the bottom & it is quite inappropriate. I'm approving as I noticed you've already raised the issue #39 which is definitely one possible way to fix that. cheers @willnwhite 👍

@snrelghgub snrelghgub added the hacktoberfest-accepted PR accepted for Hacktoberfest label Oct 20, 2020
@ghost ghost merged commit 4f375c9 into development Oct 20, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted PR accepted for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display search results
1 participant