fix: make setItems method on accordion protected for extensibility of base class#35898
Conversation
📊 Bundle size report✅ No changes found |
|
Pull request demo site: URL |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-web-components/Avatar 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-web-components/Avatar. - Dark Mode.normal.chromium_1.png | 298 | Changed |
| vr-tests-web-components/Avatar. - Dark Mode.normal.chromium.png | 10380 | Changed |
vr-tests-web-components/MenuList 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-web-components/MenuList. - RTL.2nd selected.chromium.png | 17 | Changed |
| vr-tests-web-components/MenuList. - RTL.2nd selected.chromium_3.png | 38816 | Changed |
| * @returns {void} | ||
| */ | ||
| private setItems = (): void => { | ||
| protected setItems = (): void => { |
There was a problem hiding this comment.
Ideally all of the methods that belong to the Accordion would be class methods instead of instance properties:
| protected setItems = (): void => { | |
| protected setItems(): void { |
(You'll also have to remove the semicolon on line 131)
There was a problem hiding this comment.
| protected isSingleExpandMode(): boolean { |
There was a problem hiding this comment.
| protected setSingleExpandMode(expandedItem: Element): void { |
There was a problem hiding this comment.
| protected removeItemListeners(oldValue: any): void { |
The type for oldValue should probably also be something better than any
There was a problem hiding this comment.
| public expandedChangedHandler: EventListener = (evt: Event): void => { |
There was a problem hiding this comment.
This is actually one of the specific things that should be addressed beyond marking the methods protected/public: why are the event handlers being added and removed to the child accordion items, by the accordion itself? It should either use one event on the accordion that identifies which accordion item was clicked, or the accordion items themselves should handle their own click events.
chrisdholt
force-pushed
the
users/chhol/make-accordion-set-items-protected
branch
from
94c2f7b to
1cf30b5
Compare
|
Closing in favor of a robust follow-up from @radium-v |
3 participants
Previous Behavior
Private methods prevent extensibility of the base class.
New Behavior
setItemsmethod updated to support extensibility by changing from private to protectedRelated Issue(s)