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 Logout button #388

Merged
merged 14 commits into from Nov 29, 2018

Conversation

Projects
None yet
3 participants
@T4rk1n
Copy link
Contributor

commented Nov 20, 2018

Add a logout button, performs a form post request to a logout_url.

Prop Type Default description
id string - Id of the button.
label string logout Text of the button
logout_url string - Url to submit a post logout request.
style object - style of the button.
className string - CSS class for the button.
@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

@T4rk1n T4rk1n requested a review from chriddyp Nov 20, 2018

@scjody
Copy link

left a comment

Would it make sense to automatically set the default logout_url if DASH_LOGOUT_URL exists in the environment as discussed at plotly/streambed#11817 (comment)?

I leave it up to the dash team to decide if this is worth doing, but without this all Dash Apps wanting to add a logout button on DDS will have to implement this so there's a good argument for centralizing it in dcc.

@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

@scjody

There is no python hooks for the components so we can't do it at the component level. Can add it from a dds app with dcc.LogoutButton(logout_url=os.getenv('DASH_LOGOUT_URL')).

@scjody

This comment has been minimized.

Copy link

commented Nov 21, 2018

That URL doesn't look so bad, except you'll want a default so it doesn't crash in test environments and for non-DDS users. So something like: dcc.LogoutButton(logout_url=os.getenv('DASH_LOGOUT_URL', 'https://dash.plot.ly/docs_on_logout'))

Is it impossible to check DASH_LOGOUT_URL in this function? (I don't know how LogoutButton.py gets generated so I accept that it may not be, but it seems strange that we wouldn't have control over the backend of all our components.)

@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

The python component class is auto-generated, it will get overwritten when the components are regenerated.

@scjody

This comment has been minimized.

Copy link

commented Nov 21, 2018

I realize that, but I'm surprised we don't have any control over the generation process or what goes into the component at that time.

Anyway if my suggestion is unreasonable I'm OK leaving things as they are.

@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

W could have some kind of hooks, it's just not implemented right now. I created an issue:

plotly/dash#464

@T4rk1n T4rk1n requested a review from valentijnnieman Nov 28, 2018

@T4rk1n T4rk1n force-pushed the logout-btn branch from c8f556b to f8792b1 Nov 28, 2018

className="dash-logout-frame"
>
<button
className={`dash-logout-btn ${className}`}

This comment has been minimized.

Copy link
@valentijnnieman

valentijnnieman Nov 28, 2018

Contributor

Here if className is undefined, it will add "undefined" to the class. Would be cleaner to check if className exists before appending it.

@valentijnnieman
Copy link
Contributor

left a comment

I'm getting 405 errors, saying "The method is not allowed for the requested URL.". Is there something we have to set-up in the underlying Flask server to allow for POST requests? Not sure why I'm getting these errors.

For the default url, how about adding it as a defaultProp? You could even do a GET instead of a POST if there's no url set (and therefore uses the default "https://dash.plot.ly/docs_on_logout") so that it links you to those docs.

Lastly, requesting a small change having to do with appending the className prop.

@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

@valentijnnieman

There's an example in the test:

@app.server.route('/_logout', methods=['POST'])
def on_logout():
      rep = flask.redirect('/logged-out')
      rep.set_cookie('logout-cookie', '', 0)
      return rep
@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

I'll make https://dash.plot.ly/dash-core-components/logout_button the default url and adds documentation and examples there when released.

@valentijnnieman

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

@T4rk1n Maybe add some more info (or a link to those docs) in the docstring for the component as well, so that it's more clear for users what this component is for.

@valentijnnieman

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

Great - looks good to me!

@T4rk1n T4rk1n force-pushed the logout-btn branch from fa4a26a to b42dabb Nov 29, 2018

@T4rk1n T4rk1n merged commit 074bca8 into master Nov 29, 2018

4 checks passed

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

@T4rk1n T4rk1n deleted the logout-btn branch Nov 29, 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.