-
Notifications
You must be signed in to change notification settings - Fork 151
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(masthead): adding shadow parts #11881
Conversation
Deploy preview created for package Built with commit: baa4559702212841a05d1464081daaa8ae46559b |
Deploy preview created for package Built with commit: baa4559702212841a05d1464081daaa8ae46559b |
Deploy preview created for package Built with commit: baa4559702212841a05d1464081daaa8ae46559b |
Deploy preview created for package Built with commit: baa4559702212841a05d1464081daaa8ae46559b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember, given the way these will be used we don't need to prefix each part name with the component name. E.g.
/* Redundant */
c4d-left-nav-menu-category-heading::part(left-nav-menu-category-h2) { ... }
/* Not redundant */
c4d-left-nav-menu-category-heading::part(heading) { ... }
I'd also encourage you to take a look at the markup as rendered in the browser to get a better idea of how all these pieces are actually used. While looking at class names for hints is good, they don't always necessarily match how the element is actually used. See my comment on <c4d-left-nav-menu-section>
's back button for an example of what I mean.
It looks like all of these part names can be simplified. I left some notes for how I might go about doing that for some components. Can you do another round trying to match that approach? Thanks!
packages/web-components/src/components/masthead/left-nav-menu-category-heading.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/left-nav-menu-category-heading.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/left-nav-menu-category-heading.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/left-nav-menu-category-heading.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/left-nav-menu-category-heading.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/left-nav-menu-section.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/left-nav-menu-section.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/left-nav-menu-section.ts
Outdated
Show resolved
Hide resolved
…m/carbon-for-ibm-dotcom into feat/masthead-shadow-parts
Hey guys @andy-blum @Valentin-Sorin-Nicolae @marcelojcs @m4olivei I have addressed the comments from John, can you please check it now? I have just simplified the naming convention for all the shadow parts. Thank you!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few things found. Overall nice work. Almost there!
packages/web-components/src/components/masthead/left-nav-menu-section.ts
Show resolved
Hide resolved
packages/web-components/src/components/masthead/masthead-global-bar.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/masthead-menu-button.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/megamenu-category-link.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/megamenu-category-link.ts
Outdated
Show resolved
Hide resolved
…m/carbon-for-ibm-dotcom into feat/masthead-shadow-parts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits. Will commit.
packages/web-components/src/components/masthead/masthead-menu-button.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/masthead-menu-button.ts
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/masthead/masthead-menu-button.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry found a couple more.
Related Ticket(s)
ADCMS-5170
Description
Adding shadow parts to the Masthead component so it can be styles outside of the Shadow Tree.
Snapshots updated.