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

[Menu] Add new context menu demo #17839

Merged
merged 11 commits into from Oct 18, 2019
Merged

Conversation

SarthakC
Copy link
Contributor

@SarthakC SarthakC commented Oct 11, 2019

Closes #1462

@SarthakC SarthakC marked this pull request as ready for review October 11, 2019 15:28
@SarthakC SarthakC closed this Oct 11, 2019
@SarthakC SarthakC reopened this Oct 11, 2019
@SarthakC SarthakC closed this Oct 11, 2019
docs/src/pages/components/menus/ContextMenu.js Outdated Show resolved Hide resolved
docs/src/pages/components/menus/ContextMenu.js Outdated Show resolved Hide resolved
docs/src/pages/components/menus/ContextMenu.js Outdated Show resolved Hide resolved
docs/src/pages/components/menus/ContextMenu.js Outdated Show resolved Hide resolved
docs/src/pages/components/menus/ContextMenu.js Outdated Show resolved Hide resolved
docs/src/pages/components/menus/ContextMenu.js Outdated Show resolved Hide resolved
docs/src/pages/components/menus/menus.md Outdated Show resolved Hide resolved
docs/src/pages/components/menus/ContextMenu.js Outdated Show resolved Hide resolved
docs/src/pages/components/menus/ContextMenu.js Outdated Show resolved Hide resolved
docs/src/pages/components/menus/ContextMenu.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari mentioned this pull request Oct 11, 2019
1 task
@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Oct 11, 2019
@oliviertassinari oliviertassinari changed the title Context menu [menu] Add new context menu demo Oct 11, 2019
@oliviertassinari oliviertassinari added the component: menu This is the name of the generic UI component, not the React module! label Oct 11, 2019
@oliviertassinari oliviertassinari changed the title [menu] Add new context menu demo [Menu] Add new context menu demo Oct 11, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Oct 11, 2019

No bundle size changes comparing 5d564f9...6654246

Generated by 🚫 dangerJS against 6654246

@SarthakC SarthakC requested a review from eps1lon October 11, 2019 21:35
@mbrookes
Copy link
Member

A couple of minor issues from playing with the demo:

  • Right-clicking elsewhere when the context menu is open doesn't change the menu context.
  • The menu items would benefit from being a more representative example of a context menu

docs/src/pages/components/menus/menus.md Outdated Show resolved Hide resolved
docs/src/pages/components/menus/menus.md Outdated Show resolved Hide resolved
docs/src/pages/components/menus/ContextMenu.js Outdated Show resolved Hide resolved
docs/src/pages/components/menus/ContextMenu.js Outdated Show resolved Hide resolved
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I have added a commit that focuses on minimizing the size of the demo. I think that are good. The final step for me would be to migrate the demo to TypeScript.

Also, @mercuriete has proposed to help test the component, do you think that you could have a look at https://deploy-preview-17839--material-ui.netlify.com/components/menus/#context-menu? Thanks.

@oliviertassinari oliviertassinari dismissed stale reviews from mbrookes and joshwooding October 13, 2019 17:22

updated

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Oct 15, 2019
SarthakC and others added 6 commits October 17, 2019 12:50
Co-Authored-By: Matt <github@nospam.33m.co>
Co-Authored-By: Matt <github@nospam.33m.co>
Co-Authored-By: Josh Wooding <12938082+joshwooding@users.noreply.github.com>
Co-Authored-By: Josh Wooding <12938082+joshwooding@users.noreply.github.com>
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@SarthakC As per your preference, I have migrated the demo to TypeScript.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6654246:

Sandbox Source
create-react-app Configuration
create-react-app-with-typescript Configuration

@oliviertassinari oliviertassinari merged commit 910da29 into mui:master Oct 18, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 18, 2019

@SarthakC First pull request ✌️, thank you, well done

@SarthakC
Copy link
Contributor Author

@SarthakC First pull request ✌️, thank you, well done

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context Menu
6 participants