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

feat(Menu): integration of menu web component to carbon repo #18612

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

amal-k-joy
Copy link
Contributor

@amal-k-joy amal-k-joy commented Feb 18, 2025

Contributes #18338 ,#17917

This PR is the cherry pick of Sangeethas PR from dotcom repo

Changelog

New

  • Moved the changes of Menu component from dotCom to the folder carbon-webcomponents as part of cherry pick
  • Copied the changes from packages/carbon-webcomponents folder to packages/webcomponents folder
  • committed yarn cache for @lit/context addition

Testing / Reviewing

The basic functionality is verified in local storybook

Copy link

netlify bot commented Feb 18, 2025

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit ed4d188
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/67bbfcb7a122980008e8c775
😎 Deploy Preview https://deploy-preview-18612--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Feb 18, 2025

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit ed4d188
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/67bbfcb76ad9cc0008b75d6b
😎 Deploy Preview https://deploy-preview-18612--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.97%. Comparing base (4315d4a) to head (ed4d188).
Report is 24 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #18612      +/-   ##
==========================================
+ Coverage   84.02%   84.97%   +0.95%     
==========================================
  Files         408      408              
  Lines       14494    14542      +48     
  Branches     4726     4764      +38     
==========================================
+ Hits        12178    12357     +179     
+ Misses       2149     2023     -126     
+ Partials      167      162       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amal-k-joy amal-k-joy marked this pull request as draft February 18, 2025 10:36
Copy link

netlify bot commented Feb 19, 2025

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit ed4d188
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/67bbfcb707a71200085e9839
😎 Deploy Preview https://deploy-preview-18612--v11-carbon-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@amal-k-joy amal-k-joy marked this pull request as ready for review February 24, 2025 04:41
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me! I did find a couple functional mismatches though:

  • Does not open submenu when pressing spacebar. Here I'm pressing the spacebar twice on each of them
2025-02-27.at.13.48.25-Components.Menu.-.Default.Storybook-Google.Chrome.mp4
  • same thing as above w/ the spacebar, but with selection
2025-02-27.at.13.56.16-Components.Menu.-.Default.Storybook-Google.Chrome.mp4
  • Submenu's do open when pressing enter, but focus does not move to the submenu
2025-02-27.at.13.53.01-Components.Menu.-.Default.Storybook-Google.Chrome.mp4

Comment on lines +20 to +32
.#{$prefix}--menu {
@include component-reset;
@include box-shadow;

position: fixed;
z-index: z('modal');
padding: $spacing-02 0;
background-color: $layer;
max-inline-size: 18rem;
min-inline-size: 10rem;
opacity: 0;
overflow-y: auto;
visibility: hidden;
Copy link
Member

Choose a reason for hiding this comment

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

Why is a duplication of these styles required?

.#{$prefix}--menu {
@include component-reset.reset;
@include box-shadow;
position: fixed;
z-index: z('modal');
padding: $spacing-02 0;
background-color: $layer;
max-inline-size: 18rem;
min-inline-size: 10rem;
opacity: 0;
overflow-y: auto;
visibility: hidden;
}

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.

3 participants