-
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?
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.
It is indeed expensive, but it's necessary for components that need JavaScript logic dependent on directionality (from my quick check it's e.g., ColorArea, ColorSlider, ColorWheel, Slider calculations, Menu placement).
But hey, at least this approach is more performant than the previous implementation 😛
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.
I reached out to PSWeb and they do not support RTL so they are not using dir at all.
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.
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 I was curious and pulled down the code and did a search on few areas and have some feedback 😄
- The
[trackedChildren](https://github.com/adobe/spectrum-web-components/blob/ruben/remove-dir/1st-gen/tools/theme/src/Theme.ts#L342) Set and the methods are now dead code. You can remove or keep as no-ops for backward compatibility. - There is a
[dir="rtl"]inspectrum-progress-bar.css. This is a progressive pattern but do you feel we should document this as this is the only css file. - I see that there is still a direction inheritance logic in
Theme.ts. Seems intentional but it is slightly contrasting with changes in the PR which relies on purely CSSdir(). - A changeset is required marking this as a breaking change.
- Just a thought: How about keeping
isLTRas a convenience getter for backward compatibility?
public get isLTR(): boolean {
return this.dir === 'ltr';
}
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