Skip to content

Conversation

@jeffredodd
Copy link
Contributor

@jeffredodd jeffredodd commented Jan 25, 2019

Description

In order to remove the dependency on React Router from Fundamentals-React (#115), we must refactor the the following components...

This PR is focused on Menu and does introduce breaking changes such as...

  • Removal of Link prop
  • Removal of LinkProps prop

React Router Menu Example (NEW)

<Menu>
    <MenuList>
        <MenuItem>
            <Link to='/'>Option 1</Link>
        </MenuItem>
        <MenuItem>
            <Link to='/'>Option 2</Link>
        </MenuItem>
        <MenuItem>
            <Link to='/'>Option 3</Link>
        </MenuItem>
    </MenuList>
</Menu>

Anchor via children (NEW)

<Menu>
    <MenuList>
        <MenuItem>
            <a href='/'>Option 1</a>
        </MenuItem>
        <MenuItem>
            <a href='/'>Option 2</a>
        </MenuItem>
        <MenuItem>
            <a href='/'>Option 3</a>
        </MenuItem>
    </MenuList>
</Menu>

Anchor via Props (Classic)

<Menu>
    <MenuList>
        <MenuItem url="/">Option 1</MenuItem>
        <MenuItem url="/">Option 2</MenuItem>
        <MenuItem url="/">Option 3</MenuItem>
        <MenuItem url="/">Option 4</MenuItem>
    </MenuList>
</Menu>

Copy link
Contributor

@jbadan jbadan left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

src/Menu/Menu.js Outdated

// ---------------------------------------- Menu Item ----------------------------------------
export const MenuItem = ({ url, link, isLink, separator, addon, children, onclick, className, addonProps, linkProps, urlProps, ...props }) => {
export const MenuItem = ({ url, link, isLink, separator, addon, children, onclick, className, addonProps, urlProps, ...props }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The link prop type was removed so can it be removed from here as well?

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.

Assuming the build passes, this looks good. 🚢

@jeffredodd
Copy link
Contributor Author

Fixed the issue with the build. Simply had to make sure when spreading our className we merged with consumer provided classNames as well. If anyone doesn't object i'll merge this shortly.

@jeffredodd jeffredodd merged commit 591b489 into master Jan 29, 2019
@jeffredodd jeffredodd deleted the fix/remove-react-router-dep-menu branch January 29, 2019 07:20
greg-a-smith pushed a commit that referenced this pull request Mar 5, 2019
* Saving state

* Updating menu to factor out react-router

* Update linting

* Reverting breadcrumb for easier Menu PR

* Updating Menu Component Documentation

* Remove unneeded MemoryRouter component from test

* Removing stray prop

* Second commit

* Second commit

* Updating Menu PR to spread Classnames

* Updating description
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.

4 participants