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

Simple card #12

Merged
merged 12 commits into from
Aug 29, 2019
Merged

Simple card #12

merged 12 commits into from
Aug 29, 2019

Conversation

allishultes
Copy link
Contributor

@allishultes allishultes commented Aug 21, 2019

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

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

Describe what the PR does

Adds SimpleCard component to the Storybook.

Includes:

  • Edit button that is logging as an action when clicked;
  • Delete button that is logging as an action when clicked;
  • Hyperlinked title container that is showing as an action when clicked;
  • Updated webpack.config referencing the component

Unknowns:

  • How to achieve greater styling control (and whether that's needed) without scss modules, which aren't supported out-of-the-box with Storybook;
  • Whether there's a better way to handle links

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

Ready for review and feedback please :)

Additional context

@allishultes allishultes marked this pull request as ready for review August 28, 2019 14:22
@allishultes allishultes requested a review from emettely August 28, 2019 14:25
@allishultes allishultes added the generic component A non-workspace component for the storybook label Aug 28, 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.

Mostly cool, more to refactor but at a later stage.

packages/components/SimpleCard/index.js Outdated Show resolved Hide resolved
packages/components/SimpleCard/index.js Outdated Show resolved Hide resolved
}

showLinkPath = () => {
return this.props.showLinkPath(this.props.id) || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth implementing a refactor to simplify the logic later on when you are working on the List.
showLinkPath shouldn't be passed on here as it over-complicates things.
to functionality in the LinkContainer should be taking in string of a url - and redirecting to that. Not taking in a showLinkPath function.

@allishultes allishultes merged commit ebc87d7 into master Aug 29, 2019
@allishultes allishultes deleted the SimpleCard branch August 29, 2019 14:37
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