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

Adds list component #14

Merged
merged 13 commits into from
Aug 30, 2019
Merged

Adds list component #14

merged 13 commits into from
Aug 30, 2019

Conversation

allishultes
Copy link
Contributor

Is your Pull Request request related to another issue in this repository ?

Issue: https://github.com/orgs/bbc/projects/42#card-25190014

Describe what the PR does

Adds a list component that displays SimpleCard components and filters the components with a search bar.

  • Additionally, has a small refactor for the SimpleCard component, in which the card is passed a url (a refactor of the showLinkPath function).

State whether the PR is ready for review or whether it needs extra work

The PR is ready for review — but I think this branch might need to be revisited in the future to handle editing and deleting of cards. Currently, this is performed at the page level in DPE-client because it involves interacting with the API Wrapper. However, it seems like we should be able to handle deletes from a list.

Additional context

@allishultes allishultes requested a review from emettely August 30, 2019 08:51
@allishultes allishultes added the generic component A non-workspace component for the storybook label Aug 30, 2019
Copy link
Contributor

@emettely emettely left a comment

Choose a reason for hiding this comment

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

Some additional feedback from Slack:
Running the storybook

  • I'm not sure if the description should be clickable in the card (not part of this list component)
  • Wonder if the Cards should be inside the List in the Storybook
  • Search isn't in the List component in the Storybook, is this expected?

packages/components/List/index.js Outdated Show resolved Hide resolved
packages/components/List/index.js Outdated Show resolved Hide resolved
packages/components/SimpleCard/index.js Outdated Show resolved Hide resolved
packages/components/List/index.js Outdated Show resolved Hide resolved
packages/components/List/index.js Outdated Show resolved Hide resolved
packages/components/SearchBar/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@emettely emettely left a comment

Choose a reason for hiding this comment

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

Additional review, sorry it's not in one go.

packages/components/List/index.js Outdated Show resolved Hide resolved
@allishultes
Copy link
Contributor Author

Some additional feedback from Slack:
Running the storybook

  • I'm not sure if the description should be clickable in the card (not part of this list component)
  • Wonder if the Cards should be inside the List in the Storybook
  • Search isn't in the List component in the Storybook, is this expected?
  • The description was clickable in the original designs; that seemed sort of weird to me.
  • I've moved the Search bar into the List component, but I'm leaving cards outside for now. I think otherwise we'll be nesting everything and that isn't totally the point. But I'll explore and propose a nesting hierarchy first thing next week.

@allishultes allishultes merged commit 5a342f4 into master Aug 30, 2019
@allishultes allishultes deleted the adds-list-component branch August 30, 2019 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generic component A non-workspace component for the storybook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants