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

don't override document.title if set to None #1343

Merged
merged 18 commits into from
Jul 27, 2020
Merged

don't override document.title if set to None #1343

merged 18 commits into from
Jul 27, 2020

Conversation

chriddyp
Copy link
Member

@chriddyp chriddyp commented Jul 23, 2020

Edit: This feature is now documented in http://dash.plotly.com/external-resources. Look for the title= and update_title=. See the "Customizing Dash's Document or Browser Tab Title", " Update the Document Title Dynamically based off of the URL or Tab", and "Customizing or Removing Dash's "Updating..." Message" sections.

As requested by a Dash Enterprise customer.

This allows users to override document.title during runtime with a
clientside callback or a special component
(plotly/dash-core-components#833)

Example of modifying during runtime:

import dash
import dash_html_components as html
import dash_core_components as dcc
from dash.dependencies import Input, Output

app = dash.Dash(__name__, update_title=None)


app.layout = app.layout = html.Div([
    dcc.Input(id='input'),
    html.Div(id='output'),

    html.Div(id='dummy'),
    dcc.Tabs(id='tabs-example', value='tab-1', children=[
        dcc.Tab(label='Tab one', value='tab-1'),
        dcc.Tab(label='Tab two', value='tab-2'),
    ]),
    html.Div(id='tabs-example-content')
])


app.clientside_callback(
    """
    function(tab_value) {
        console.log(tab_value);
        document.title = tab_value;
        return null; // dummy output
    }
    """,
    Output('dummy', 'children'),
    [Input('tabs-example', 'value')]
)


@app.callback(Output('output', 'children'), [Input('input', 'value')])
def update_output(value):
    return value

if __name__ == '__main__':
    app.run_server(debug=True)

Example with custom table title via index string:

import dash
import dash_html_components as html
import dash_core_components as dcc
from dash.dependencies import Input, Output

app = dash.Dash(__name__, update_title=None)

app.index_string = '''
<!DOCTYPE html>
<html>
    <head>
        {%metas%}
        <title>Hello World</title>
        {%favicon%}
        {%css%}
    </head>
    <body>
        <div>My Custom header</div>
        {%app_entry%}
        <footer>
            {%config%}
            {%scripts%}
            {%renderer%}
        </footer>
        <div>My Custom footer</div>
    </body>
</html>
'''


app.layout = app.layout = html.Div([
    dcc.Input(id='input'),
    html.Div(id='output'),

    html.Div(id='dummy'),
    dcc.Tabs(id='tabs-example', value='tab-1', children=[
        dcc.Tab(label='Tab one', value='tab-1'),
        dcc.Tab(label='Tab two', value='tab-2'),
    ]),
    html.Div(id='tabs-example-content')
])


@app.callback(Output('output', 'children'), [Input('input', 'value')])
def update_output(value):
    return value

if __name__ == '__main__':
    app.run_server(debug=True)

This is a follow up of #1315.

This allows users to override `document.title` during runtime with a
clientside callback or a special component
(plotly/dash-core-components#833)
update_title,
};
}

UNSAFE_componentWillReceiveProps(props) {
if (this.state.update_title && props.isLoading) {
document.title = this.state.update_title;
} else {
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Jul 24, 2020

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 inititalTitle if the value is equal to update_title instead? Possibly the same chekc for update_title - only update if the value is currently initialTitle.

Copy link
Member Author

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_title meant. I believe e190cc9 is the logic we're looking for

Copy link
Collaborator

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.isLoading test) if !update_title. Then this entire component would be a noop, as it should be in that case; callbacks can do whatever they want with document.title and 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 changes document.title, but we're still in a loading state so DocumentTitle doesn't receive new props and we don't pick up the changed title into this.state. Then after the second callback we reset document.title to this.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.title below it would be better to do something like:

if (document.title === this.state.update_title) {
    document.title = this.state.title;
} else {
    this.setState({title: document.title});
}

Copy link
Member Author

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.

@chriddyp
Copy link
Member Author

Huh, odd that pylint would fail. I didn't change any python files.
image

@alexcjohnson
Copy link
Collaborator

Tangentially related to this PR - you can set app.title to control the regular page title (which I totally forgot about when reviewing #1315). We use this internally, but I don't see it documented anywhere, and it feels inconsistent with the update_title kwarg. Perhaps we should support title as a kwarg too? Also app.config.update_title works, so it would be nice if app.config.title did too. We could go full symmetry and allow app.update_title, but it might be cleaner to deprecate app.title and encourage both of these to go through kwargs or app.config.

@alexcjohnson
Copy link
Collaborator

alexcjohnson commented Jul 24, 2020

Huh, odd that pylint would fail.

weird, perhaps we somehow got isort v5? Maybe we need to pin it to 4.3.21 in requires_dev.txt? pylint tries to handle that itself, but perhaps someone else brings it in unpinned first?

update: yes, somehow we have isort@5.1.0

️️🏗️ pip dev requirements
...
Requirement already satisfied: isort>=4.2.5 in ./venv/lib/python3.6/site-packages (from pylint==1.9.4->-r requires-dev.txt (line 7)) (5.1.0)

@chriddyp
Copy link
Member Author

Perhaps we should support title as a kwarg too?

I added this as well and kept the app.title for backwards compatibility. Also added a unit test here.

@chriddyp
Copy link
Member Author

Huh, seeing these re-introduced snapshots on Percy

image

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle we could make title into a @property that syncs app.title with app.config.title - but not a big deal. Most people should just use the kwarg regardless.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 💃

@chriddyp chriddyp merged commit 3435639 into dev Jul 27, 2020
@chriddyp chriddyp deleted the document-title branch July 27, 2020 15:43
@alexcjohnson
Copy link
Collaborator

Dunno what the deal is with the "reintroduced" percy snapshots - but I'd just ignore it this time and look into it if we see that again.

@OverLordGoldDragon
Copy link

Isn't below simpler?

def interpolate_index(self, **kwargs):
    kwargs['title'] = 'Custom Title'
    return dash.Dash.interpolate_index(self, **kwargs)
app.interpolate_index = interpolate_index.__get__(app)

@alexcjohnson
Copy link
Collaborator

I'm not sure what you mean @OverLordGoldDragon - simpler than app = dash.Dash(title='Custom Title')?

@OverLordGoldDragon
Copy link

@alexcjohnson Surely not, but your code blob isn't shown in this PR nor in Releases (it was unclear where the "title parameter" should go) - thanks for clarifying.

@alexcjohnson
Copy link
Collaborator

OIC - yeah, this PR expanded a bunch from its original purpose, so we never showed the simple examples - sorry for the confusion. We've documented the new behavior at https://dash.plotly.com/external-resources (though @chriddyp it feels to me a bit hidden on that page - maybe time for a dedicated dash.Dash class API reference page?)
As we've discussed, this is a big argument in favor of merging the docs into this repo - then https://github.com/plotly/dash-docs/pull/931 would have been part of this PR.

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.

4 participants