refactor(multiple): rework LabelControl across aria components#33146
refactor(multiple): rework LabelControl across aria components#33146adolgachev wants to merge 3 commits into
Conversation
|
Deployed dev-app for a867429 to: https://ng-dev-previews-comp--pr-angular-components-33146-dev-qadmum20.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
3082e63 to
bad3cc9
Compare
There was a problem hiding this comment.
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.
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.
| }); | ||
|
|
||
| constructor() { | ||
| this._labelControl = new LabelControl({ |
There was a problem hiding this comment.
We should put this in the UI pattern to keep the directive a thin layer between the framework and the core library.
There was a problem hiding this comment.
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.
|
This draft PR is being closed because it has been stale for 28 days and has seen no activity from you. If you'd like to see this change land, you can re-open this PR. Thank you for being an Angular contributor! |
Refactors to make LabelControl easier to use by directives directly:
Added proper usage to AccordionPanel and TabPanel so default labelledby still set properly to the respective AccordionTrigger or Tab, but also could be overridden. Although not recommended to do so.