-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Configure document-title during updates #1315
Conversation
@stlehmann thanks, this looks great! Re tests: How about in |
Co-authored-by: alexcjohnson <johnson.alex.c@gmail.com>
Thanks for the hint. I'll get into it. |
Co-authored-by: alexcjohnson <johnson.alex.c@gmail.com>
Co-authored-by: alexcjohnson <johnson.alex.c@gmail.com>
@alexcjohnson I added tests to test_loading_states.py. I tested for update_title being set to default, None, empty and a custom string. Hopefully the test implementation is acceptable. Otherwise just let me know :) |
with lock: | ||
dash_duo.start_server(app) | ||
dash_duo.find_element("#button").click() | ||
assert dash_duo.driver.title == "Updating..." |
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.
Very nice tests! One thing though: your callback will execute on page load, right? So holding the lock while starting the server means we're testing the title during the initialization sequence. That's a good thing to test, but I think in at least one of these tests (and maybe all of them) we should test:
- during startup, we see the correct updating title
- when the callback is done we see the regular title
- when the callback is triggered interactively we see the updating title again
Also rather than a simple assert
, for robustness I think it'd be better to do a wait.until
, in case things don't happen quite in the expected order:
from dash.testing.wait import until
until(lambda: dash_duo.driver.title == "Updating...", timeout=1)
And one last thing, if you're feeling adventurous: These test cases are nearly identical so they could be parametrized, something like:
import pytest
@pytest.mark.parametrize(
"kwargs,page_updating_title",
[
({}, "Updating..."),
({"updating_title": None}, "Dash"),
({"updating_title": ""}, "Dash"),
({"updating_title": "Hello World"}, "Hello World")
]
)
def test_rdls003_update_title_default(dash_duo, updating_title, page_updating_title)
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 tried to implement your suggestions. I omitted an explicit test for the correct update title on startup because the callback should be called on startup afaik. I hope this is OK. The parametrize decorator is a great thing. Didn't know that until now, thanks :) Hope I did it all right with the locks. I'm not that sure about it.
def update(n): | ||
with lock: | ||
# check for update-title while processing callback | ||
until(lambda: dash_duo.driver.title == expected_update_title, timeout=1) |
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.
Interesting... this appears to work but I'm kind of surprised it does - it might actually be waiting for the timeout period and then throwing an error silently. That's because the server and the browser side are running in different processes (hence the need for Lock
).
I'd remove the title check inside the callback, and just put it at the very end of the test, after #button.click()
(but still inside the with lock:
block)
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'd remove the title check inside the callback, and just put it at the very end of the test, after #button.click() (but still inside the with lock: block)
This was my first shot but that didn't work at that time because I missed the lock inside the callback. Now this works like a charm. I also added the check on startup again.
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.
Beautiful! Thanks for the test updtes. I'm not sure what happened with the Percy run but that's not on your side, just something flaky - I'll sort it out. 💃
You are very welcome. Thanks again for the review and the great mentoring. 🙇 |
}; | ||
} | ||
|
||
UNSAFE_componentWillReceiveProps(props) { | ||
if (props.isLoading) { | ||
document.title = 'Updating...'; |
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.
@Marc-Andre-Rivet as we were discussing the other day - the renderer no longer holds this default value at all. That means when the R and Julia implementations update to include this functionality in their renderer, they will need to add this option to the back end at the same time or the default behavior will change to no title changes ever.
Update to those passing through GitHub: This feature is now documented in http://dash.plotly.com/external-resources. Look for the |
This PR addresses #856 and #732. It adds an
update_title
parameter toDash.__init__()
which can be set to a String or None. If this parameter is set to a String this will be the document title that is shown if Dash is updating. Ifupdate_title
is set to None or "" the document title will not change at all. This is especially useful for applications with short update intervals to prevent flickering of the document title.I have not yet added any tests because I didn't know where to put them. If you should request some tests I am more than willing to provide some :)