Skip to content

Conversation

@cjskillingstad
Copy link
Contributor

Description

  • Separated all components in the Menu directory into their own files.
  • Maintained all git history.

jbadan
jbadan previously requested changes Feb 23, 2019
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.

blocking until fundamental-bot is reinstated #435

@jbadan jbadan dismissed their stale review February 24, 2019 00:52

bot is reinstated

src/index.js Outdated
export { default as Menu } from './Menu/Menu';
export { default as MenuList } from './Menu/MenuList';
export { default as MenuItem } from './Menu/MenuItem';
export { default as MenuGroup } from './Menu/MenuGroup';
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have expected to see all these as static references on Menu and then only Menu would be exported. Something like:

Menu.Item = MenuItem;
Menu.Group = MenuGroup;
Menu.List = MenuList;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, should've caught that. Moved to subcomponents in 8127fcc

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

@cjskillingstad cjskillingstad merged commit 3aecc51 into master Feb 27, 2019
@cjskillingstad cjskillingstad deleted the fix/one_comp_menu branch February 27, 2019 17:59
greg-a-smith pushed a commit that referenced this pull request Mar 5, 2019
* Rename file

* Rename temp

* Restore original

* Rename file

* Rename temp

* Restore original

* Rename file

* Rename temp

* Restore original

* Separate components

* Move Menu subcomponents

* Update snapshot with correct on click function
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