Skip to content

FIX #673 - CardFooter component#700

Merged
dej611 merged 2 commits intoitalia:masterfrom
luco5826:master
Apr 16, 2021
Merged

FIX #673 - CardFooter component#700
dej611 merged 2 commits intoitalia:masterfrom
luco5826:master

Conversation

@luco5826
Copy link
Copy Markdown
Contributor

Fixes #673

PR Checklist

  • My branch is up-to-date with the Upstream master branch.
  • The unit tests pass locally with my changes
  • I have added necessary documentation (if appropriate)

Short description of what this resolves:

The new CardFooter component is a wrapper for the class it-card-footer provided by BI.

Changes proposed in this pull request:

  • New component in Cards' storybook (showing the CardFooter usage)

@dej611
Copy link
Copy Markdown
Member

dej611 commented Apr 11, 2021

Hi @luco5826 , thanks for your first contribution!

I'm reviewing this PR but I'm a bit concerned with the renaming bit. I guess the original issue wasn't clear enough, so I'm sorry for that.
Renaming a component may be a breaking change - as small as it could be - in this context, so I would probably ask if it's possible to restore the previous version of the component CardFooterCTA ( bad name, I agree on that ) and keep the story, maybe with a better name? Something like "Card con footer": what do you think?

@luco5826
Copy link
Copy Markdown
Contributor Author

luco5826 commented Apr 11, 2021

Hi @dej611 !

You're completely right, I didn't think about the renaming being a breaking change and in fact I was a bit worried about that.

I'm going to fix it asap and since we're here, do you think it is better to create a CardFooter component as I implemented it (as a simple div) or as a wrap around the existing CardFooter component from reactstrap?

[EDIT]: I fixed those things you mentioned and I squashed all my commits into a single meaningful one, let me know if that is ok or I should rework it in another way!

Copy link
Copy Markdown
Member

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Left a note.
Everything else is ok, but I need that change to approve it 👍

Comment on lines +6 to +13
className: PropTypes.string,
children: PropTypes.node
}

const CardFooterCTA = props => {
const { className, ...attributes } = props
const { className, children } = props
const classes = classNames(className, 'it-card-footer')
return <div className={classes} {...attributes} />
return <div className={classes}>{children}</div>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you restore it with the old version?

The problem with the new one is that you are only letting the children props pass. Say the developer wants to add an aria-something on the div, how can he does that?

attributes contains children and also many HTML attributes in it.

@luco5826
Copy link
Copy Markdown
Contributor Author

@dej611 Hi!
I've restored the previous version of CardFooterCTA but atm I really do not understand what was the point about this issue.
The component already existed and it was implemented in the correct way, so the only feature was to add it to the documentation, right?

@dej611
Copy link
Copy Markdown
Member

dej611 commented Apr 16, 2021

Yes. True.
As said above the original issue was misleading and not well explained.
I think your work on the documentation is great and will definetly help developers!

@dej611 dej611 merged commit 51438e4 into italia:master Apr 16, 2021
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.

[FEATURE] Create new CardFooter component

2 participants