feat(masthead): adding shadow parts #11881
Conversation
bruno-amorim
requested review from
ariellalgilmore,
IgnacioBecerra,
m4olivei,
jkaeser,
marcelojcs,
andy-blum and
Valentin-Sorin-Nicolae
and removed request for
a team
|
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.
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!! |
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
m4olivei
requested review from
jkaeser
and removed request for
ariellalgilmore,
IgnacioBecerra and
jkaeser
…m/carbon-for-ibm-dotcom into feat/masthead-shadow-parts
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
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.