-
Notifications
You must be signed in to change notification settings - Fork 6.8k
refactor(multiple): rework LabelControl across aria components #33146
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ import { | |
| input, | ||
| signal, | ||
| } from '@angular/core'; | ||
| import {TabPattern, TabPanelPattern, DeferredContentAware} from '../private'; | ||
| import {DeferredContentAware, LabelControl, TabPattern, TabPanelPattern} from '../private'; | ||
| import {TABS} from './tab-tokens'; | ||
|
|
||
| /** | ||
|
|
@@ -49,7 +49,8 @@ import {TABS} from './tab-tokens'; | |
| '[attr.id]': '_pattern.id()', | ||
| '[attr.tabindex]': '_pattern.tabIndex()', | ||
| '[attr.inert]': '!visible() ? true : null', | ||
| '[attr.aria-labelledby]': '_pattern.labelledBy()', | ||
| '[attr.aria-label]': '_labelControl.label()', | ||
| '[attr.aria-labelledby]': '_labelControl.labelledBy()', | ||
| }, | ||
| hostDirectives: [ | ||
| { | ||
|
|
@@ -71,9 +72,18 @@ export class TabPanel implements OnInit, OnDestroy { | |
| /** The parent Tabs. */ | ||
| private readonly _tabs = inject(TABS); | ||
|
|
||
| /** Controls label for this tabpanel. */ | ||
| readonly _labelControl: LabelControl; | ||
|
|
||
| /** A global unique identifier for the tab. */ | ||
| readonly id = input(inject(_IdGenerator).getId('ng-tabpanel-', true)); | ||
|
|
||
| /** The (optional) label for the accordion panel. */ | ||
| readonly label = input<string | undefined>(undefined); | ||
|
|
||
| /** The (optional) labelledBy ids for the accordion panel. */ | ||
| readonly labelledBy = input<string[]>([]); | ||
|
|
||
| /** The Tab UIPattern associated with the tabpanel */ | ||
| readonly _tabPattern: WritableSignal<TabPattern | undefined> = signal(undefined); | ||
|
|
||
|
|
@@ -90,6 +100,12 @@ export class TabPanel implements OnInit, OnDestroy { | |
| }); | ||
|
|
||
| constructor() { | ||
| this._labelControl = new LabelControl({ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should put this in the UI pattern to keep the directive a thin layer between the framework and the core library.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the reason I pulled it out is we don't have to bring back AccordionPanelPattern just to serve as a wrapper for this utility. |
||
| defaultLabelledBy: computed(() => this._pattern?.labelledBy()), | ||
| label: this.label, | ||
| labelledBy: this.labelledBy, | ||
| }); | ||
|
|
||
| afterRenderEffect({ | ||
| write: () => { | ||
| this._deferredContentAware.contentVisible.set(this.visible()); | ||
|
|
||
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.
Can we stop the "I want this in my own style" kind of refactoring? I don't see the core functionality has changed after touching almost every single line.
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.
See the PR comments for the 3 benefits in usage. If we go to the trouble of creating a utility, it should be easy and clear in its usage. Otherwise, could literally just do the logic more simply in the pattern. In this case, it turned out to be premature re-factoring as the LabelControl was not used in any beneficial way previously, just adding to burden.