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 dontReenableAfterSuccess prop to support other default states bes… #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

codylieu
Copy link

@codylieu codylieu commented Feb 8, 2020

…ides enabled

There seems to be a bug where the currentState state value and the state prop fall out of sync. This manifested itself in my project when the default state of the button (what it should get set to after success according to the onSuccess prop) is something other than ''.

The code already partially supports not automatically setting the button to enabled after success. This PR adds a prop which can be used to configure this. I tested this in my project and it works as expected when my code sets the button state to 'disabled' after success.

@codylieu
Copy link
Author

codylieu commented Feb 8, 2020

Attaching an image of the inconsistency between state prop and currentState state. I'm using react hooks / onSuccess prop to set the button state to 'disabled' after it's state is 'success'. Not too familiar with react / js still so let me know if there's something I'm missing

image

@codylieu codylieu force-pushed the allow-default-states-besides-enabled-after-success branch from dc030d6 to 5922e31 Compare February 8, 2020 07:48
@mathieudutour
Copy link
Owner

I'm confused, don't you want to set controlled to true instead?

@codylieu
Copy link
Author

codylieu commented Feb 9, 2020

@mathieudutour Sorry, I do have controlled set to true, my screenshot above was a bit incorrect. Here's a correct screenshot that shows controlled=true with the inconsistency between the state prop and the currentState state attribute:
image

More details in case they're helpful:

After I put my button in the success state, I have the following function passed to the onSuccess prop:

const resetRecordMetricButtonOnSuccess = useCallback(() => {
        setButtonState('disabled')
}, [])

I've noticed that the button becomes disabled as expected, but then becomes enabled immediately afterwards. I've looked over my code fairly carefully and don't see anything that would be setting it to enabled after the disabled state.

I think this is happening because the state prop (disabled) falls out of sync with the currentState state attribute ('') as shown in the picture above.

I commented out the following code in the ProgressButton class and it fixed my issue:

if (!dontRemove) {
        _this2.setState({ currentState: STATE.NOTHING });
}

This is what led me to add this dontReenableAfterSuccess prop. Let me know if I might be missing something. Also let me know if there are more details I can share to help you understand my issue

@mathieudutour
Copy link
Owner

maybe the fix would be to change if (!dontRemove) { this.setState({currentState: STATE.NOTHING}) } to if (!dontRemove && !this.props.controlled) { this.setState({currentState: STATE.NOTHING}) }? That seems like what we want in a controlled state.

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.

None yet

2 participants