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 update_title config option #102

Merged
merged 4 commits into from
May 7, 2021

Conversation

etpinard
Copy link
Collaborator

@etpinard etpinard commented May 5, 2021

fixes #101

  • add update_title String field to DashConfig struct
  • use `"Updating..." as default
  • inject its value into the frontend, to make it available to the dash renderer

TODO:

  • should we allow update_title=nothing inputs?
  • add tests

- add `update_title` String field to DashConfig struct
- use `"Updating..." as default
- inject its value into the frontend, to make it available
  to the dash renderer
Comment on lines +285 to +289
- `update_title::String`: Default ``Updating...``. Configures the document.title
(the text that appears in a browser tab) text when a callback is being run.
Set to '' 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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Only supporting '' is fine by me.

@alexcjohnson
Copy link
Contributor

re: tests - dynamic behavior is covered by the tests in plotly/dash#1315, here it would be fine to just assert dashjl.driver.execute_script("return store.getState().config.update_title") == expected for the default and one non-default case.

@etpinard
Copy link
Collaborator Author

etpinard commented May 6, 2021

here it would be fine to just assert dashjl.driver.execute_script("return store.getState().config.update_title") == expected for the default and one non-default case.

My attempt in -> 4b07cab

Thanks for tips!

@alexcjohnson
Copy link
Contributor

LGTM! I've enabled forks in CircleCI, didn't realize we hadn't done that yet. Would you push an empty commit to see if the tests will run?

@alexcjohnson
Copy link
Contributor

@waralex This step fails for forks:

Pkg.add(PackageSpec(url="https://github.com/plotly/Dash.jl.git", rev=ENV["CIRCLE_BRANCH"]))

But we already have the appropriate version of the code in a local directory, can't we just use that?

@waralex
Copy link
Collaborator

waralex commented May 7, 2021

@waralex This step fails for forks:

Pkg.add(PackageSpec(url="https://github.com/plotly/Dash.jl.git", rev=ENV["CIRCLE_BRANCH"]))

But we already have the appropriate version of the code in a local directory, can't we just use that?

Yes, we can use Pkg.develop instead of Pkg.add

 Pkg.develop(pkg::Union{String, Vector{String}}; io::IO=DEFAULT_IO[])
  Pkg.develop(pkgs::Union{Packagespec, Vector{Packagespec}}; io::IO=DEFAULT_IO[])

  Make a package available for development by tracking it by path. If pkg is given with only a name or by a URL, the package will be downloaded to the location specified by the environment variable JULIA_PKG_DEVDIR, with .julia/dev as the default.

  If pkg is given as a local path, the package at that path will be tracked.

  Examples
  ≡≡≡≡≡≡≡≡≡≡

  # By name
  Pkg.develop("Example")

  # By url
  Pkg.develop(url="https://github.com/JuliaLang/Compat.jl")

  # By path
  Pkg.develop(path="MyJuliaPackages/Package.jl")

  See also PackageSpec

@waralex
Copy link
Collaborator

waralex commented May 7, 2021

@alexcjohnson, @etpinard In fact, there is another problem - the components are placed by direct reference to git, and not through the package repository. Apparently I wrote this file when the components were not registered yet. I fixed it in this PR #104. In that #104 unit tests pass, let's check in current one. Perhaps you should just copy the changes from #104 to this PR and merge them within current PR

@waralex waralex mentioned this pull request May 7, 2021
Copy link
Contributor

@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.

Great - Thanks @etpinard ! 💃

@alexcjohnson alexcjohnson merged commit d27478f into plotly:dev May 7, 2021
@etpinard etpinard deleted the add-update_title-config-option branch May 7, 2021 21:17
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.

Document title Updating... broken?
3 participants