-
Notifications
You must be signed in to change notification settings - Fork 241
chore(base): remove dir management from SpectrumElement and sp-theme
#5936
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
base: main
Are you sure you want to change the base?
Conversation
|
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
58c9ada to
55fcc7c
Compare
dir management from SpectrumMixin
dir management from SpectrumMixindir management from SpectrumElemen
dir management from SpectrumElemendir management from SpectrumElement
dir management from SpectrumElementdir management from SpectrumElement and sp-theme
|
Solid modernization effort, we just cleaned up 4+ years of workaround code. |
| static VERSION = version; | ||
|
|
||
| public override get dir(): CSSStyleDeclaration['direction'] { | ||
| return getComputedStyle(this).direction ?? 'ltr'; |
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.
None of our components read dir during construction phase so we are safe but this is a bit expensive. Can you work with some partner teams and check if this is something worth auditing?
Rajdeepc
left a comment
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.
Nothing blocking.Coverage report flagging a few missing branches. If you could take a look at those and either add coverage or confirm they’re intentional, I’m happy to come back for a re-review.
Description
This refactor removes the legacy JavaScript-based
dir(text direction) management fromSpectrumElementand replaces it with the modern CSS:dir()pseudo-class. The previous implementation used aMutationObserverto watch fordirattribute changes ondocument.documentElementand propagated these to child components. This approach required explicitdirattributes on elements and manual registration withsp-theme.The new approach leverages the browser's native
:dir()CSS pseudo-class, which automatically inherits directionality from the DOM hierarchy without requiring explicit attributes. This reduces JavaScript overhead, simplifies the component architecture, and improves performance.Key changes:
isLTRgetter anddirParenttracking fromSpectrumMixinconnectedCallback/disconnectedCallbackoverrides that managed direction registrationMutationObserverandobservedForElementsSet for tracking direction changesdirgetter usinggetComputedStyle(this).directionfor JavaScript access when needed[dir="ltr"]/[dir="rtl"]attribute selectors with:dir(ltr)/:dir(rtl)Note:
The only expected VRT failures are due to a fix for a previously unknown
sp-linkRTL issue affecting multiple components.Motivation and context
This refactor is in preparation for 2nd-gen components, which will not include this shared direction management logic. By moving to the CSS
:dir()pseudo-class now, we ensure a consistent approach across both 1st-gen and 2nd-gen components, and reduce the surface area of shared code that needs to be maintained between generations.The CSS
:dir()pseudo-class is now well-supported across modern browsers and provides a more performant, declarative approach to handling text direction. It automatically inherits directionality from parent elements in the DOM hierarchy, eliminating the need for manual attribute management and JavaScript-based observation.Related issue(s)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesDevice review