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 1 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/Badge 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-web-components/Badge. - Dark Mode.normal.chromium.png | 443 | Changed |
vr-tests-web-components/MenuList 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-web-components/MenuList. - RTL.2nd selected.chromium.png | 17 | Changed |
vr-tests-web-components/RadioGroup 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-web-components/RadioGroup. - Dark Mode.2nd selected.chromium_3.png | 119 | 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)
| * Checks if the accordion is in single expand mode | ||
| * @returns {boolean} | ||
| */ | ||
| private isSingleExpandMode(): boolean { |
There was a problem hiding this comment.
| protected isSingleExpandMode(): boolean { |
| * @param expandedItem The item to expand in single expand mode | ||
| * @returns {void} | ||
| */ | ||
| private setSingleExpandMode(expandedItem: Element): void { |
There was a problem hiding this comment.
| protected setSingleExpandMode(expandedItem: Element): void { |
| * Removes event listeners from the previous accordion items | ||
| * @param oldValue An array of the previous accordion items | ||
| */ | ||
| private removeItemListeners = (oldValue: any): 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
| * @param evt Click event | ||
| * @returns | ||
| */ | ||
| private expandedChangedHandler: EventListener = (evt: Event): void => { |
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.
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)