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

Delegate displaying of page titles to Layout.display() #4578

Merged
merged 18 commits into from Sep 12, 2017

Conversation

Projects
None yet
4 participants
@dannon
Copy link
Member

commented Sep 8, 2017

This is another angle on #4473.

Instead of having the analysis app handle setting this stuff, this delegates to the active layout's display method. This approach separates concerns a bit better -- when writing a new interface, you just set a title attribute on what gets display()'ed (right now, backbone Views) if it should have a page title, and it should all work regardless of layout. Right now, only the Page layout exists, but you could imagine each layout's display method doing different things as appropriate with view titles.

Fixes #4416

@dannon dannon added the status/WIP label Sep 8, 2017

@dannon dannon added this to the 17.09 milestone Sep 8, 2017

@dannon dannon requested a review from guerler Sep 8, 2017

@dannon dannon added the area/UI-UX label Sep 8, 2017

Galaxy.display = this.display = function( view ) { self.center.display( view ) };
Galaxy.display = this.display = function( view ) {
if ( view.title ){
window.document.title = "Galaxy | " + view.title;

This comment has been minimized.

Copy link
@martenson

martenson Sep 8, 2017

Member

would this be the place to include the app.config.brand if present? (as in #4272)

This comment has been minimized.

Copy link
@dannon

dannon Sep 8, 2017

Author Member

Yep, I like that. I was also talking to @guerler about whether or not we should just set it to Galaxy | <brand> if there is no view title set. I'm looking for cases where we may not want to do that, and if I can't find one I'll add that too.

@@ -20,6 +20,7 @@ return Backbone.View.extend({
initialize: function(grid_config) {
this.grid = new GridModel();
this.dict_format = grid_config.dict_format;
this.title = grid_config.title;

This comment has been minimized.

Copy link
@dannon

dannon Sep 8, 2017

Author Member

This doesn't actually work all the time because grid_config.title is shipped from many of the dict endpoints, which get initialized and rendered async. Need to look into a more robust solution.

This comment has been minimized.

Copy link
@dannon

dannon Sep 11, 2017

Author Member

(handled this for now by setting a delayed title initialization flag indicating to views that the title hasn't been set yet and that they can set the window title on their own)

@@ -91,7 +91,7 @@ define( [ 'mvc/form/form-view', 'mvc/ui/ui-misc', 'utils/query-string-parsing' ]

/** View of the main user preference panel with links to individual user forms */
var View = Backbone.View.extend({

title: "User Preferences",

This comment has been minimized.

Copy link
@anuprulez

anuprulez Sep 9, 2017

Member

Can we apply localization for these title texts as well just the way we do it for menu?

This comment has been minimized.

Copy link
@dannon

dannon Sep 9, 2017

Author Member

Good idea, that should be easy to add!

This comment has been minimized.

Copy link
@dannon

dannon Sep 9, 2017

Author Member

(and, done in 928ad29)

This comment has been minimized.

Copy link
@anuprulez

anuprulez Sep 9, 2017

Member

thanks a lot 👍

@dannon dannon added status/review and removed status/WIP labels Sep 11, 2017

dannon added some commits Sep 11, 2017

Grids do a late initialization and the title isn't available until th…
…is happens (after display()), so provide a way to allow views to set title late if necessary.
When title isn't set, revert to the default -- otherwise display()'d …
…contents without titles show up with the previous and incorrect titles.

@dannon dannon added status/WIP and removed status/review labels Sep 11, 2017

@dannon dannon added status/review and removed status/WIP labels Sep 11, 2017

Workflow editor title fix (not new-style, to keep it simple and use t…
…he approach consistent with the .mako layout used.
@dannon

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2017

Ok, should handle everything mentioned in #4416 along with many other problems now.

@dannon dannon removed the request for review from guerler Sep 12, 2017

@guerler guerler merged commit 2e16093 into galaxyproject:dev Sep 12, 2017

6 checks passed

api test Build finished. 290 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 45 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
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.