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

feat: button toggle: Allow consumer to control pressed state #5143

Merged
merged 6 commits into from
Nov 12, 2024

Conversation

margaree
Copy link
Contributor

@margaree margaree commented Nov 11, 2024

Jira ticket

Problem:
Previously in the MVC toggle control, the on click event could happen prior to the pressed state being changed. With the switch to the component, that no longer happens. This was found to be an issue in discussions, where the consumer needs to click the button > accept a dialog > then the state should change.

Fix notes:
This allows the consumer to manage the state rather than managing the state through a click handler. Let me know alternative ideas and also alternative property names. I wasn't sure if there was another property with a similar function to model the name off of. Removed new property and switched to an event approach instead

To do:

  • add vdiff test(s) if this approach seems good

Copy link
Contributor

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://live.d2l.dev/prs/BrightspaceUI/core/pr-5143/

Note

The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

@margaree margaree marked this pull request as ready for review November 11, 2024 16:33
@margaree margaree requested a review from a team as a code owner November 11, 2024 16:33
@@ -36,4 +37,65 @@ describe('button-toggle', () => {

});

describe('consumer manages state', () => {
Copy link
Member

Choose a reason for hiding this comment

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

These feel like they might be better suited to normal unit tests that just assert that the value of elem.pressed is what you're expecting.

@@ -2,6 +2,7 @@ import { css, html, LitElement } from 'lit';

/**
* A button container component for button toggles.
* @fires click - Internal event
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem possible to @ignore our new click event since it's assigned to a variable, so this is our hack for not showing it in the docs (see this)

components/button/test/button-toggle.test.js Outdated Show resolved Hide resolved
components/button/test/button-toggle.test.js Outdated Show resolved Hide resolved
components/button/test/button-toggle.test.js Outdated Show resolved Hide resolved
@margaree margaree merged commit 9eed635 into main Nov 12, 2024
6 checks passed
@margaree margaree deleted the button-toggle-consumer-handle-pressed branch November 12, 2024 15:12
Copy link

🎉 This PR is included in version 3.66.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@margaree
Copy link
Contributor Author

Since this ends up triggering the click event listener on the button twice it's ending up a bit tricky LMS-side and in the meantime the double onclick is not a great behaviour (e.g., in discussions it's opening a double dialog). I'm going to rethink this a bit but might revert in the meantime (though it's possible no one one notice the weird behaviour)

@dbatiste
Copy link
Contributor

I was thinking you might have to add a classInitializer and manage things from there instead of wiring up their functions to ButtonIcon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants