-
Notifications
You must be signed in to change notification settings - Fork 44
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
Fix computation error on disclosure (take 2 π) #461
Conversation
π¦ Changeset detectedLatest commit: 71465b3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
639fb83
to
dddbf0c
Compare
dddbf0c
to
093595b
Compare
093595b
to
4a39977
Compare
|
||
export default class HdsDisclosureComponent extends Component { | ||
@tracked isOpen; // notice: if in the future we need to add a "@isOpen" prop to control the status from outside (eg to have the Disclosure opened on render) just add "this.args.isOpen" here to initalize the variable | ||
@tracked toggleRef; | ||
@tracked isToggleClicked; |
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.
This is a leftover from an old refactoring β not used anywhere across the repo.
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.
I don't see anything on my side, but I am not an Ember expert, so I would suggest to ask an extra couple of people with strong Ember background for review (or even better, post the PR in the #tech-frontend Slack channel for second opinions).
4a39977
to
91ce0da
Compare
91ce0da
to
f9b38f4
Compare
f9b38f4
to
143b7b6
Compare
143b7b6
to
2706b9f
Compare
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.
OK for me
2706b9f
to
71465b3
Compare
π Summary
Fix computation error on disclosure utility component.
π οΈ Detailed description
In #456 I wrongly identified the issue β the solution made the
cloud-ui
tests pass, but only sometimes. After managing to replicate the failing tests locally and testing with https://github.com/hashicorp/cloud-ui/pull/3098 it turns out the racing happens between the@action
functions (onFocusOut
,onKeyUp
) triggering the samenotifyPropertyChange
viaclose
in a short amount of time (even shorter in automated tests). I tried many options to prevent this error from happening in tests without avail. The only solution I could find that prevents tests incloud-ui
from failing intermittently is to move what happens in theclose
function to the next runloop β hence I'm proposing this to unblock the adoption of this component until a wiser solution may be determined.Preview
Failing/ο¬aky test in HCP consul
Before
After
π How to review
π Review by files changed
Reviewer's checklist:
π¬ Please consider using conventional comments when reviewing this PR.