-
Notifications
You must be signed in to change notification settings - Fork 42
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(button): restore disabled state set from outside when loading sta… #130
fix(button): restore disabled state set from outside when loading sta… #130
Conversation
@EndzeitBegins, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction. |
|
||
it('should go to enabled set while loadingState was non-default, when it changes back', async () => { | ||
await componentIsStable(component); | ||
component.disabled = true; // should NOT be restored, as it is changed while in non-default loadingState |
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'm confused by this comment, by default the loadingState is the default loading state
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.
Can you keep the existing names and implementations for these unit tests. I think it's helpful to have tests where the implicit behavior happens - ie, no one ever set the disabled status before mucking with the loading states
Thanks for taking a stab at this, I'm reviewing and looking through it now. I don't love the idea of completely disregarding the value attempting to be set in the setter. We already had the Likewise, I'm not sure I follow your unit tests updates and what they're attempting to do that's different than what they did before. |
@EndzeitBegins, VMware has approved your signed contributor license agreement. |
Thank you for taking a look at my proposal @ashleyryan. There were two tests which ensured that the disabled state (true / false), which is set while the However, I added two tests which reproduce the problem I originally encountered using the Angular integration. As I'm able to reproduce this without Angular being involved, I'm quite certain this is a bug in the core framework. I'm haven't worked with Also, in my proposal the Trying to summarize the difference: // Assuming defaults
disabled = false;
loadingState = "default";
// Use case 1 - setting before loadingState / with default
disabled = true;
loadingState = "loading";
loadingState = "default"; // return back to "default"
-> disabled is true (which is correct imo)
// Use case 2 - setting after loadingState / with non-default
loadingState = "loading";
disabled = true;
loadingState = "default"; // return back to "default", restores value before loadingState changed
-> disabled is false (which is incorrect imo) |
|
||
// do NOT propagate disabled state, unless we're in default loading state | ||
// otherwise button can be enabled while being in a non-default loading state, e.g. loading | ||
if (this.isDefaultLoadingState(this.loadingState)) { |
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 was playing around with my own implementation loosely based off of yours and I was able to get around having this conditional logic here. I think we can always call super
here and it still more or less works the same.
} else { | ||
this.disableButton(); | ||
} | ||
if (this.isDefaultLoadingState(this.loadingState)) { |
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'm trying to remember why, but I think there was a reason I had to have the undefined check here in the last PR. Otherwise I think disabled gets called if other prop values change - we only want to run this is loadingState is the prop changing
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 remember why - props always had loadingState, but unless it's being changed, it's undefined. I found that without the undefined check, the else statement was being called on the initial render when loadingState
was set, but the button hadn't been painted to the dom yet so the width was 0px.
I added a test for this situation.
This is causing some of the react snapshot/unit tests to fail and I'm investigating why. |
Thank you @ashleyryan for investing time looking into this issue. Should I change my pull-request in any way? Or do you simply take over the issue? I've found a slightly different solution that also passes the added tests. But still fails the aforementioned snapshot tests, as The idea is to basically use the get disabled(): boolean {
return super.disabled || !this.isDefaultLoadingState(this.loadingState);
}
set disabled(value: boolean) {
super.disabled = value;
} Coupled with notifying update(props: PropertyValues<this>) {
if (props.has('loadingState')) {
// change in loadingState might also affect disabled state
this.requestUpdate('disabled', super.disabled);
if (this.isDefaultLoadingState(this.loadingState)) {
this.style.removeProperty('width');
} else {
this.style.width = getElementWidth(this);
}
}
super.update(props);
} With this approach, the override of |
I can either take the PR over or you can try and make some of the changes you or I have suggested. Why don't you try updating your commit to have the alternative implementation, I like that approach. We also need to update the commit message to include a link to the original issue. So update it to something like
I've spent most of the day digging into why lit is doing that with boolean attributes and I've been talking to them. I've narrowed it down to something not working correctly when using a getter or setter vs a simple property. I think it's fine and I can update the react tests if necessary. But let's try the updated solution for now. I'm also happy to take it over, if you'd like me to. |
I ended up taking it over @EndzeitBegins . I couldn't get your suggested approach to work - even though the tests were passing I was finding that in storybook when I was manipulating the component with
That it wasn't losing the disabled status for some reason. |
Thanks for looking into this and finding a solution that works with the remaining use cases, @ashleyryan. I hope my initial test cases were helpful in aiding the way at least. Looking forward to the merge! |
… default - previously disabled state was only tracked when set when loading state was default - now the state is tracked when set during loading state changes too fixes #129 Signed-off-by: EndzeitBegins <16666115+EndzeitBegins@users.noreply.github.com>
🎉 This PR is included in version 6.1.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
- adopts the fix for boolean getters and setters from lit/lit#2799 (comment) - fixes an issue introduced in PR vmware-clarity#130 where cds-pagination-button was getting disabled="false" rendered
- adopts the fix for boolean getters and setters from lit/lit#2799 (comment) - fixes an issue introduced in PR vmware-clarity#130 where cds-pagination-button was getting disabled="false" rendered
- adopts the fix for boolean getters and setters from lit/lit#2799 (comment) - fixes an issue introduced in PR vmware-clarity#130 where cds-pagination-button was getting disabled="false" rendered
- adopts the fix for boolean getters and setters from lit/lit#2799 (comment) - fixes an issue introduced in PR #130 where cds-pagination-button was getting disabled="false" rendered
Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed PRs after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary. |
…te changes to default
Signed-off-by: EndzeitBegins 16666115+EndzeitBegins@users.noreply.github.com
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #129
What is the new behavior?
When changing to the
loadingState
to default thedisabled
value set from outside is restored, whether is was set before or while theloadingState
was non-default.Does this PR introduce a breaking change?
Other information
My solution includes replacing the field
disabled
inCdsBaseButton
with a corresponding getter and setter in order to override its behavior inCdsButton
. This in turn adds a private field_disabled
toCdsBaseButton
. To avoid collisions I renamed_disabled
to_disabled_from_outside
inCdsButton
.According to
npm run public-api:check
this results in changes to the "public" API, even though only private fields were added / renamed and the fielddisabled
replaced with getter / setter.