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

Fix display of ConfirmDialogProvider if not used in a callback. #241

Merged
merged 4 commits into from Aug 13, 2018

Conversation

Projects
None yet
3 participants
@T4rk1n
Copy link
Contributor

commented Jul 23, 2018

  • Add support to use the ConfirmDialog and ConfirmDialogProvider without a callback assigned to it.
  • Fix callbacks sent to both submit and cancel at the same time.

Fix #242.

@T4rk1n T4rk1n requested a review from valentijnnieman Jul 23, 2018

@bpostlethwaite
Copy link
Member

left a comment

Perhaps a test for the non-callback functionality is warranted? If we can't use Percy to snapshot Dialogues perhaps some sort of unit test will suffice.

this.state = {
displayed: props.displayed
};
this._setStateAndProps = this._setStateAndProps.bind(this);

This comment has been minimized.

Copy link
@bpostlethwaite

bpostlethwaite Jul 23, 2018

Member

I don't see _setStateAndProps being passed as a callback anywhere so this .bind() isn't necessary?

};
this._setStateAndProps = this._setStateAndProps.bind(this);
this._update = this._update.bind(this);
// this._update();

This comment has been minimized.

Copy link
@bpostlethwaite

@T4rk1n T4rk1n force-pushed the confirm-dialog-state branch from 9015d91 to a6b0278 Jul 24, 2018

@chriddyp

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

@T4rk1n are you planning on adding tests to this one as per @bpostlethwaite 's recommendation? Or are confirm dialog tests too hard to add with selenium?

Also, does this fix #242?

This PR is blocking the documentation plotly/dash-docs#123, so it'd be good to get it in soon 😺

@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2018

@chriddyp Yes, I just didn't have the time to do it, I will fix #242 in this also, think I have the fix in stash. I'll do it soon.

@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2018

I added tests for ConfirmDialog without callbacks and fixed #242.

@chriddyp please review.

@chriddyp

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

💃 thanks!

@T4rk1n T4rk1n force-pushed the confirm-dialog-state branch from 0f96813 to 1c9fd82 Aug 13, 2018

@T4rk1n T4rk1n merged commit 895d3e3 into master Aug 13, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
percy/dash-core-components No visual changes since last approval
Details

@T4rk1n T4rk1n deleted the confirm-dialog-state branch Aug 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.