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: props spreading to Menu #236

Merged
merged 3 commits into from
Jan 17, 2019
Merged

fix: props spreading to Menu #236

merged 3 commits into from
Jan 17, 2019

Conversation

chrismanciero
Copy link
Contributor

Add props spreading to the Menu component

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.

{
name: 'titleProps',
description: 'object - additional props to be spread to the Menu Group title'
},
Copy link
Contributor

Choose a reason for hiding this comment

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

The addonProps and linkProps are actually for the MenuItem component and the titleProps are actually for the MenuGroup 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 Author

Choose a reason for hiding this comment

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

How about something like this?

screen shot 2019-01-17 at 11 55 37 am

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's a good start -- certainly good enough for this PR. 👍 I created #238 for a more widespread change across all component pages for a later date.

src/Menu/Menu.js Outdated Show resolved Hide resolved
src/Menu/Menu.js Outdated
{children}
</Link>
}
{url &&
<a className={`fd-menu__item${isLink ? ' fd-menu__link' : ''}`} href={url}>
<a {...linkProps} className={`fd-menu__item${isLink ? ' fd-menu__link' : ''}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do <Link> and <a> need separate props. Maybe a urlProps for the latter? I'm not quite sure why both exist either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added new prop - urlProps for elements

src/Menu/Menu.js Outdated Show resolved Hide resolved
<MemoryRouter>
<Menu>
<MenuList>
<MenuItem linkProps={{ 'data-sample': 'Sample' }} url='/' />
Copy link
Contributor

Choose a reason for hiding this comment

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

Per comment above, should this be a new prop (urlProps)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New prop - urlProps added

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. 🚢

@chrismanciero chrismanciero merged commit d51988c into master Jan 17, 2019
@chrismanciero chrismanciero deleted the fix/menu-spread branch January 17, 2019 17:40
greg-a-smith pushed a commit that referenced this pull request Mar 5, 2019
* fix: props spreading to Menu

* added urlProps spread

* update to documentation for Menu subcomponents
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

2 participants