Skip to content
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

Accordion default slot #5514

Merged
merged 7 commits into from
Jan 20, 2022
Merged

Conversation

m4thieulavoie
Copy link
Collaborator

Pull Request

📖 Description

This PR addresses the issue in accordion where it's unclear whether the slot name is "item" or "". We now default to a default slot, while maintaining backward compatibility.
Fixes #5437

🎫 Issues

#5437

👩‍💻 Reviewer Notes

📑 Test Plan

✅ Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

⏭ Next Steps

@@ -15,10 +15,7 @@ export const accordionItemTemplate: (
context: ElementDefinitionContext,
definition: AccordionItemOptions
) => html`
<template
class="${x => (x.expanded ? "expanded" : "")}"
slot="item"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out FAST was automatically adding the slot under the hood. I assume only a tiny portion of the consumers will have to keep their eyes open for a breaking change, as long as they use a component extending AccordionItem

@@ -14,8 +14,9 @@ export const accordionTemplate: (
) => ViewTemplate<Accordion> = (
context: ElementDefinitionContext,
definition: FoundationElementDefinition
) => html`
) => /* TODO: deprecate slot name `item` to only support default slot https://github.com/microsoft/fast/issues/XXXX */ html`
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrisdholt did you want to create the issue, or do you want me to, and you would add any additional details afterwards?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind taking care of this? Thank you!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Member

@chrisdholt chrisdholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @m4thieulavoie for this!

@radium-v radium-v merged commit 415e7a4 into microsoft:master Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: inconsistent default slot naming in accordian item
5 participants