Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 2.3k
don't override document.title if set to None#1343
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
66bb5d32dcc00ae190cc9f8216b9104510e84266d75da53ce219a50be2a29118e80ad95f36766864b1fb09014a43a61fd4710269d057dca791a4aa5d587495File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -227,6 +227,15 @@ class Dash(object): | ||
| with a ``plug`` method, taking a single argument: this app, which will | ||
| be called after the Flask server is attached. | ||
| :type plugins: list of objects | ||
| :param title: Default ``Dash``. Configures the document.title | ||
| (the text that appears in a browser tab). | ||
| :param update_title: Default ``Updating...``. Configures the document.title | ||
| (the text that appears in a browser tab) text when a callback is being run. | ||
| Set to None or '' if you don't want the document.title to change or if you | ||
| want to control the document.title through a separate component or | ||
| clientside callback. | ||
| """ | ||
| def __init__( | ||
| @@ -252,6 +261,7 @@ def __init__( | ||
| prevent_initial_callbacks=False, | ||
| show_undo_redo=False, | ||
| plugins=None, | ||
| title='Dash', | ||
| update_title="Updating...", | ||
| **obsolete | ||
| ): | ||
| @@ -300,6 +310,7 @@ def __init__( | ||
| ), | ||
| prevent_initial_callbacks=prevent_initial_callbacks, | ||
| show_undo_redo=show_undo_redo, | ||
| title=title, | ||
| update_title=update_title, | ||
| ) | ||
| self.config.set_read_only( | ||
| @@ -321,6 +332,9 @@ def __init__( | ||
| "via the Dash constructor" | ||
| ) | ||
| # keep title as a class property for backwards compatability | ||
| self.title = title | ||
| # list of dependencies - this one is used by the back end for dispatching | ||
| self.callback_map = {} | ||
| # same deps as a list to catch duplicate outputs, and to send to the front end | ||
| @@ -725,7 +739,9 @@ def index(self, *args, **kwargs): # pylint: disable=unused-argument | ||
| config = self._generate_config_html() | ||
| metas = self._generate_meta_html() | ||
| renderer = self._generate_renderer() | ||
| title = getattr(self, "title", "Dash") | ||
| # use self.title instead of app.config.title for backwards compatibility | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In principle we could make title into a | ||
| title = self.title | ||
| if self._favicon: | ||
| favicon_mod_time = os.path.getmtime( | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
While this allows the clientside callback to set the value, it seems to break existing tests / behavior. Would a better verification be to only revert back to
inititalTitleif the value is equal toupdate_titleinstead? Possibly the same chekc forupdate_title- only update if the value is currentlyinitialTitle.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.
Thanks! I totally misinterpreted what
update_titlemeant. I believe e190cc9 is the logic we're looking forThere 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 think it'd be safer to bail out entirely (before even the
props.isLoadingtest) if!update_title. Then this entire component would be a noop, as it should be in that case; callbacks can do whatever they want withdocument.titleand we won't interfere.When you do have an
update_title, I may agree with @Marc-Andre-Rivet that there needs to be a test when we're reverting back to the non-updating title. Consider a callback chain, where the first callback changesdocument.title, but we're still in a loading state soDocumentTitledoesn't receive new props and we don't pick up the changed title intothis.state. Then after the second callback we resetdocument.titletothis.state.title, losing the callback-provided title.Seems to me the logic you have right now is fine during loading, but when reverting to
this.state.titlebelow it would be better to do something like: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.
Thanks @alexcjohnson@Marc-Andre-Rivet ! I believe I have addressed your comments and added the appropriate tests now.