8000 kmdc-menu and kmdc-menu-surface by pocharlebois · Pull Request #67 · mpetuska/kmdc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

kmdc-menu and kmdc-menu-surface #67

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jan 9, 2022
Merged

kmdc-menu and kmdc-menu-surface #67

merged 9 commits into from
Jan 9, 2022

Conversation

pocharlebois
Copy link
Contributor
@pocharlebois pocharlebois commented Jan 5, 2022 •
edited by mpetuska
Loading

Provides support for MDCMenu and MDCMenuSurface
Demonstrate use as a dropdown in Sandbox

Fixes #18
Fixes #17

@pocharlebois
kmdc-menu and kmdc-menu-surface
7dbb6cb 
Provides support for MDCMenu and MDCMenuSurface
Demonstrate use as a dropdown in Sandbox

Fixes #18 and #19
mpetuska
mpetuska requested changes Jan 5, 2022
View reviewed changes
Copy link
Owner
@mpetuska mpetuska left a comment

Choose a reason for hiding this comment

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

Some work needed. I recommend reviewing MDC docs with a fresh set of eyes as well as having a look at newer KMDC modules to get a feel at how wrapping should be done.

mpetuska
mpetuska requested changes Jan 6, 2022
View reviewed changes
Copy link
Owner
@mpetuska mpetuska left a comment

Choose a reason for hiding this comment

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

Much better this time around.

P.S. Conversations should be resolved by their author, not comitter. Also I'd very much appreciate if you could resist force-pushes as it invalidates some review history for me and makes incremental reviews harder.

@mpetuska mpetuska self-assigned this Jan 6, 2022
@pocharlebois
Copy link
Contributor Author

@mpetuska I did another pass which should resolve all pending issues.

mpetuska
mpetuska requested changes Jan 7, 2022
View reviewed changes
Copy link
Owner
@mpetuska mpetuska left a comment

Choose a reason for hiding this comment

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

Some new things to look into as well as some things remaining unresolved from the previous review.

mpetuska
mpetuska requested changes Jan 8, 2022
View reviewed changes
Copy link
Owner
@mpetuska mpetuska left a comment

Choose a reason for hiding this comment

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

Almost there. Main leftover concern is Corner "enum"

mpetuska
mpetuska approved these changes Jan 9, 2022
View reviewed changes
Copy link
Owner
@mpetuska mpetuska left a comment

Choose a reason for hiding this comment

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

We got there in the end. There are still few minor inconsistencies, but I can sort them out myself during final pre-0.1.0 review. Thanks!

@mpetuska mpetuska merged commit 3ba4633 into mpetuska:master Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpetuska mpetuska mpetuska approved these changes

Assignees

@mpetuska mpetuska

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrap mdc-menu Wrap mdc-menu-surface
2 participants
@pocharlebois @mpetuska
0