refactor(platform): consolidate and expose shadow dom detection utilities #18582
Conversation
crisbeto
added
P2
googlebot
added
the
cla: yes
crisbeto
force-pushed
the
shadow-dom-utilities
branch
from
430d397 to
cafafbb
Compare
crisbeto
force-pushed
the
shadow-dom-utilities
branch
2 times, most recently
from
0b72e73 to
9bd9660
Compare
| if (shadowDomIsSupported == null) { | ||
| const head = typeof document !== 'undefined' ? document.head : null; | ||
| shadowDomIsSupported = !!(head && ((head as any).createShadowRoot || head.attachShadow)); | ||
| } | ||
|
|
||
| return shadowDomIsSupported; | ||
| } | ||
|
|
||
| /** Gets the shadow root of an element, if supported and the element is inside the Shadow DOM. */ | ||
| export function getShadowRoot(element: HTMLElement): Node | null { |
There was a problem hiding this comment.
Is cdk/platform the right place for this? I'm not sure we have a package that really makes sense for it currently. I would think something like cdk/dom would be the right place
There was a problem hiding this comment.
I personally could see it live in something like cdk/platform/dom.
There was a problem hiding this comment.
I'm extremely hesitant to get into the business of general DOM helper APIs. I think it's a slippery slope in terms of adding more, and they tend to go out of date over the years. I'm okay with this living here if we keep it as a private API.
There was a problem hiding this comment.
It's not really private if we export it at the top level though. Also we're already in the business of detecting other DOM APIs like supported input types and scroll behavior so I don't see how detecting if the Shadow DOM is supported is any different.
There was a problem hiding this comment.
We have a cdk/testing/private, maybe we could make cdk/platform/private (I'm less concerned about it being in this package if its private)
There was a problem hiding this comment.
I'd be fine with putting it under private. The main concern I'm trying to address with these changes is the code duplication.
mmalerba
added
target: minor
|
changing to |
crisbeto
force-pushed
the
shadow-dom-utilities
branch
2 times, most recently
from
4a42bfc to
a333883
Compare
…ties Over time we had accumulated the same utility for finding the shadow root of an element in multiple places. These changes move it into a common place under `cdk/platform`. I also decided to remove the underscore from `_supportsShadowDom` so it's officially a public API. It was already exposed under `cdk/platform` so there was nothing stopping people from using it, but the underscore looked weird. The API should be stable enough, since we've had it for more than a year in multiple components and we haven't had issues with it.
crisbeto
force-pushed
the
shadow-dom-utilities
branch
from
a333883 to
4ad46c0
Compare
crisbeto
added
target: patch
mmalerba
added
pr: lgtm
action: merge
…ties (#18582) Over time we had accumulated the same utility for finding the shadow root of an element in multiple places. These changes move it into a common place under `cdk/platform`. I also decided to remove the underscore from `_supportsShadowDom` so it's officially a public API. It was already exposed under `cdk/platform` so there was nothing stopping people from using it, but the underscore looked weird. The API should be stable enough, since we've had it for more than a year in multiple components and we haven't had issues with it. (cherry picked from commit fa96c8a)
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Over time we had accumulated the same utility for finding the shadow root of an element in multiple places. These changes move it into a common place under
cdk/platform.Note: I'm setting this to a P2, because it's required to make more shadow DOM-related fixes without duplicating the logic.