Menu : deprecate props.mode / always support full capabilities + icons #18703
Conversation
resolved changes
…nto menuWC_parity
amal-k-joy
changed the title
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18703 +/- ##
==========================================
- Coverage 84.97% 84.34% -0.64%
==========================================
Files 408 408
Lines 14542 14645 +103
Branches 4764 4771 +7
==========================================
- Hits 12357 12352 -5
- Misses 2024 2132 +108
Partials 161 161 ☔ View full report in Codecov by Sentry. |
| // @provide({ context: MenuContext }) | ||
| // @consume({ context: MenuContext, subscribe: true }) | ||
| // contextChild; |
There was a problem hiding this comment.
Can we clean up the commented out code if it's not needed?
| //this.context.updateFromChild?.({ hasIcons: true },this); | ||
| // if(!this.context.hasIcons) |
There was a problem hiding this comment.
| //this.context.updateFromChild?.({ hasIcons: true },this); | |
| // if(!this.context.hasIcons) |
| } else { | ||
| // this.hasIcon=false; | ||
| } |
There was a problem hiding this comment.
| } else { | |
| // this.hasIcon=false; | |
| } | |
| } |
| [`${prefix}--menu-item`]: true, | ||
| [`${prefix}--menu-item--disabled`]: isDisabled, | ||
| [`${prefix}--menu-item--danger`]: isDanger, | ||
| // [`${prefix}--menu-item--with-icon`]:!this.hasIcon |
There was a problem hiding this comment.
| // [`${prefix}--menu-item--with-icon`]:!this.hasIcon |
| ${isRtl ? CaretLeft16() : CaretRight16()} | ||
| </div> | ||
| <cds-menu | ||
| .isChild="${hasChildren}" |
There was a problem hiding this comment.
If hasChildren is a boolean we should be able to apply the attribute or not based on its value
| .isChild="${hasChildren}" | |
| ?isChild="${hasChildren}" |
| <cds-menu-item-radio-group | ||
| label="Font weight" | ||
| items={items} | ||
| selectedItem={items[3]} | ||
| itemToString={(item) => item.label} | ||
| />; |
There was a problem hiding this comment.
| <cds-menu-item-radio-group | |
| label="Font weight" | |
| items={items} | |
| selectedItem={items[3]} | |
| itemToString={(item) => item.label} | |
| />; | |
| <cds-menu-item-radio-group | |
| label="Font weight" | |
| items={items} | |
| selectedItem={items[3]} | |
| itemToString={(item) => item.label}></cds-menu-item-radio-group>; |
| import FolderShared16 from '@carbon/icons/lib/folder--shared/16.js'; | ||
| import Cut16 from '@carbon/icons/lib/cut/16.js'; | ||
| import Copy16 from '@carbon/icons/lib/copy/16.js'; | ||
| //import TrashCan16 from '@carbon/icons/lib/trash-can/16.js'; |
There was a problem hiding this comment.
| //import TrashCan16 from '@carbon/icons/lib/trash-can/16.js'; |
| const subitemlist = ['None', 'Product team', 'Organization', 'Company']; | ||
| return html` | ||
| <cds-menu ?open=${open} size=${size}> | ||
| <cds-menu-item label="Share with" .renderIcon=${FolderShared16}> |
There was a problem hiding this comment.
Instead of passing in the icon through the renderIcon property, can the icon be slotted in instead? Having to pass in the icon this way might be difficult for folks using CDN or are not using Lit in their own application.
We can detect if icon has been passed in the slot using the _handleSlotChange() event (ex: https://github.com/carbon-design-system/carbon/blob/main/packages/web-components/src/components/text-input/text-input.ts#L52-L75)
|
@amal-k-joy with the 2 other Menu PRs (#18718 and #18612), can you clarify which one we should be focusing on first? |
Contributes #18338
Adopt changes from jans PR for deprecating mode
Changelog
New
Changed
Removed
Testing / Reviewing
{{ Add descriptions, steps or a checklist for how reviewers can verify this PR works or not }}