chore(web-components): use predicate function instead of instanceof in Accordion #32822
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Bundle size report✅ No changes found |
🕵 fluentui-web-components-v3 No visual regressions between this PR and main |
radium-v
force-pushed
the
users/radium-v/accordion-item-type-predicate
branch
from
7c2686d
to
23d7954
Compare
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
marchbox
approved these changes
Sep 13, 2024
| * | ||
| * @public | ||
| */ | ||
| export function isAccordion(element: Element): element is Accordion { |
There was a problem hiding this comment.
Should element be HTMLElement instead?
davatron5000
requested changes
Sep 25, 2024
Comment on lines
8
to
+9
| export { styles as accordionStyles } from './accordion.styles.js'; | ||
| export { definition as accordionDefinition } from './accordion.definition.js'; | ||
| export { template as accordionTemplate } from './accordion.template.js'; |
There was a problem hiding this comment.
Capitalize AccordionStyles and AccordionTemplate - Also in PR #32075
| (item: Element | BaseAccordionItem) => item instanceof BaseAccordionItem && item.expanded, | ||
| ) ?? this.accordionItems[0] | ||
| ); | ||
| private findExpandedItem(): BaseAccordionItem | null { |
There was a problem hiding this comment.
I think based on the changes this can't return null anymore, only undefined?
|
This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous Behavior
In systems which bundle components individually, we noticed that by using
instanceofin theAccordionclass, we're pulling theBaseAccordionclass into our bundles.New Behavior
This PR introduces type predicate functions,
isAccordionItemandisAccordion, to cheaply determine if a child component is one which is expected. These functions do this by checking if thetagNameends with abaseNamevalue, soisAccordionItem(element)will return true ifelement.tagNameends with "accordion-item".Some other changes:
index.tsexportsaccordionDefinitionandaccordionItemDefinition. These should be deprecated and removed in favor of the more consistent PascalCasedAccordionDefinitionandAccordionItemDefinition.Accordion.accordionItemsproperty was typed asElement[], which required a lot of checking to confirm if the collection actually containsAccordionItem[]. This PR fixes the property's type to make it easier to work with.This PR also fixes some minor API Extractor warnings generated by the
AccordionandAccordionItemmodules.