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

add active id to accordion change event #6131

Merged

Conversation

chrisdholt
Copy link
Member

Pull Request

πŸ“– Description

Adds information on the activeid for the accordion as part of the elements custom event.

🎫 Issues

closes #6130

πŸ‘©β€πŸ’» 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

@chrisdholt chrisdholt changed the title Users/chhol/add active id to accordion change event add active id to accordion change event Jun 20, 2022
Copy link
Contributor

@EisenbergEffect EisenbergEffect left a comment

Choose a reason for hiding this comment

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

Is this consistent with the way we handle it in other components? I don't recall seeing our code using the id in the event args before.

@chrisdholt
Copy link
Member Author

chrisdholt commented Jun 20, 2022

Is this consistent with the way we handle it in other components? I don't recall seeing our code using the id in the event args before.

As @vnbaaij calls out, tabs does this exact thing. The delta here from other components is that activeid is internal to accordion and private, so passing it as part of the event should allow us to expose it gracefully.

this.$emit("change", this.activetab);

@EisenbergEffect EisenbergEffect self-requested a review June 20, 2022 17:54
Copy link
Contributor

@EisenbergEffect EisenbergEffect left a comment

Choose a reason for hiding this comment

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

Probably need to port this to the archive branch as well.

"comment": "add activeid to accordion change event",
"packageName": "@microsoft/fast-foundation",
"email": "chhol@microsoft.com",
"dependentChangeType": "patch"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to change this to "prerelease" also.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

Dangit - I missed pushing this...PR incoming

@vnbaaij
Copy link
Contributor

vnbaaij commented Jun 20, 2022

After looking at the source again, I see the accordion-item emits the change event too. And that one has a public id. As he events bubble up (if I understood correctly), maybe it makes more sense to change it in the accordion-item itself?

@chrisdholt
Copy link
Member Author

chrisdholt commented Jun 20, 2022

After looking at the source again, I see the accordion-item emits the change event too. And that one has a public id. As he events bubble up (if I understood correctly), maybe it makes more sense to change it in the accordion-item itself?

Eh, we could, but I wonder if that's enough in and of itself - that value is already available publicly in the event object I believe.

@vnbaaij - you might be able to get the event target in this scenario; I actually think this is a better and more aligned approach considering tabs and the fact that I want the activeid for accordion itself.

@scomea
Copy link
Collaborator

scomea commented Jun 23, 2022

After looking at the source again, I see the accordion-item emits the change event too. And that one has a public id. As he events bubble up (if I understood correctly), maybe it makes more sense to change it in the accordion-item itself?

This is related. #6144 to "accordion" reacting to "change" events from items other than "accordion-item" . I have a local branch that adds checks to "accordion" to ensure it only reacts to these events from direct children but I do wonder if we should have more specific event names rather than so many "change" events (ie. "expanded").

@chrisdholt chrisdholt merged commit ee4b085 into master Jun 28, 2022
@chrisdholt chrisdholt deleted the users/chhol/add-active-id-to-accordion-change-event branch June 28, 2022 17:38
chrisdholt added a commit that referenced this pull request Oct 24, 2022
* add activeid to accordion change event

* Change files
chrisdholt added a commit that referenced this pull request Oct 24, 2022
* add activeid to accordion change event

* Change files
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: custom 'change' event has no arguments in fast-accordion
5 participants