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

fix: update prop spreading for ActionBar #219

Merged
merged 3 commits into from
Jan 17, 2019
Merged

Conversation

jbadan
Copy link
Contributor

@jbadan jbadan commented Jan 16, 2019

Adds additional props to enable spreading into button, h1 and p elements.

@jbadan jbadan requested a review from a team January 16, 2019 17:42
Copy link
Contributor

@greg-a-smith greg-a-smith left a comment

Choose a reason for hiding this comment

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

Looks good. 🚢

<p className='fd-action-bar__description'>{description} </p>
<h1
className='fd-action-bar__title'
{...titleProps}>{title}</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

After looking at this some more, I think we want the spread to occur first and then the hard-coded props. Otherwise, consumers could literally override required prop values. In these two cases, it will also leverage some capabilities of the classnames story to make sure they can include additional classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - I'll need to fix this on the other pr's too

Copy link
Contributor

@greg-a-smith greg-a-smith left a comment

Choose a reason for hiding this comment

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

Merging blocked until fundamental-bot admin rights are restored.

Copy link
Contributor

@greg-a-smith greg-a-smith left a comment

Choose a reason for hiding this comment

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

I think the changes look good. 🚢 I can approve once merging is allowed.

{
name: 'buttonProps',
description: 'object - additional props to be spread to the ActionBarBack\'s button.'
},
Copy link
Contributor

@greg-a-smith greg-a-smith Jan 17, 2019

Choose a reason for hiding this comment

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

The buttonProps are actually for the ActionBarBack component. That said, I would like to circle back in another story and adjust the documentation pages to properly define all the components AND sub components AND each of their respective props better.

See #238

Copy link
Contributor

@greg-a-smith greg-a-smith left a comment

Choose a reason for hiding this comment

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

Let's roll 🎢

@cjskillingstad cjskillingstad merged commit bd642f3 into master Jan 17, 2019
@cjskillingstad cjskillingstad deleted the fix/actionbar-spread branch January 17, 2019 19:41
greg-a-smith pushed a commit that referenced this pull request Mar 5, 2019
* update prop spreading for ActionBar

* move spreads

* update documentation with new props
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.

None yet

3 participants