-
Notifications
You must be signed in to change notification settings - Fork 13
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
Use single method to open/close flyout #542
base: main
Are you sure you want to change the base?
Conversation
}; | ||
|
||
_hideFlyout = (e) => { | ||
this.opened = false; | ||
this._close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lit provides a native way to update dependent attributes/variables on reactive property changes. We should use that pattern instead of relying on manually calling methods like this. With willUpdate()
, the current code would be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at the tests I wrote and try to write a test case for this case. These cases where we need to interact with the elements to do QA are pretty hard to notice after deploy otherwise.
Cool... I'm a little lost in general with all of this, so might take a while to get back to this. |
@ericholscher let me know if I can help here or if you want me to take over it. |
Feel free to run with it. It was just a small bug I wanted to fix, but the PR feedback seems to require a ton of research that I don't have energy for currently. |
Fixes #541