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

Replacing Edit Attributes Mako by JavaScript #4334

Merged
merged 23 commits into from Aug 1, 2017

Conversation

Projects
None yet
6 participants
@anuprulez
Copy link
Member

commented Jul 23, 2017

This Pull Request replaces the Mako file for the edit attributes feature of dataset by JavaScript.

edit

Details:

It uses the Galaxy's user interface elements like Ui Tabs and Form builder to build the view replacing the mako. As soon as the attributes are updated, the left-side History panel refreshes with the updated values. In addition to this, these attributes are updated asynchronously (without loading the entire page).

Thanks a lot to @guerler and @bgruening!

All suggestions are welcome!

@bgruening

This comment has been minimized.

Copy link
Owner

commented on 7ba4f22 Jul 16, 2017

Nice thanks!

@galaxybot galaxybot added the triage label Jul 23, 2017

@galaxybot galaxybot added this to the 17.09 milestone Jul 23, 2017

@bgruening

This comment has been minimized.

Copy link
Member

commented Jul 23, 2017

Cool! Thanks a lot!

@bgruening bgruening added area/UI-UX and removed triage labels Jul 24, 2017

error : function( response ) {
var error_response = { 'status': 'error',
'message': 'Error occured while saving. Please fill all the required fields and try again.' };
self.display_message( self, error_response, self.$el.find( '.edit-attr' ) );

This comment has been minimized.

Copy link
@guerler

guerler Jul 24, 2017

Contributor

I think we should Ui.Message instead of this custom message rendering if possible.


initialize: function( dataset_id ) {
this.setElement( '<div/>' );
this.render( dataset_id );

This comment has been minimized.

Copy link
@guerler

guerler Jul 24, 2017

Contributor

The render function should not require a parameter. Instead we could populate a model e.g. this.model = new Backbone.Model( options ) in the initialize method and then use the model in the render function.

/** Dataset edit attributes view */
var View = Backbone.View.extend({

initialize: function( dataset_id ) {

This comment has been minimized.

Copy link
@guerler

guerler Jul 24, 2017

Contributor

Instead of just parsing dataset_id I suggest to parse a regular dictionary with a dataset_id attribute.

@guerler

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

Thanks a lot for this contribution. It looks very good, there only a few issues we could improve. I commented above. Another thing I noticed is that the dataset id is not parsed properly if I click on the edit button of a dataset in the history i.e. the wrong dataset is selected for attribute editing in the center panel.

@@ -174,6 +176,10 @@ window.app = function app( options, bootstrapped ){
this.page.display( new CustomBuilds.View() );
},

show_dataset_edit_attributes : function() {
this.page.display( new DatasetEditAttributes.View( Utils.getQueryString( 'dataset_id' ) ) );

This comment has been minimized.

Copy link
@guerler

guerler Jul 24, 2017

Contributor

I noticed that we use something like Galaxy.params.dataset_id in other locations. It might be better to use that here too. In the future we should be able to turn this global variable into a local one.

'type' : 'text',
'label': 'Name:',
'value': data.get_display_name(),
'size' : '40'

This comment has been minimized.

Copy link
@guerler

guerler Jul 24, 2017

Contributor

Size attribute is not used anymore.

@@ -401,24 +403,26 @@ def __ok_to_edit_metadata( dataset_id ):
message = 'Attributes have been queued to be updated'

This comment has been minimized.

Copy link
@guerler

guerler Jul 24, 2017

Contributor

Add punctation for consistency.

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2017

@guerler Thanks for the review!

define( [ 'utils/utils', 'mvc/ui/ui-tabs', 'mvc/ui/ui-misc', 'mvc/form/form-view' ], function( Utils, Tabs, Ui, Form ) {

// Model for the view
var Model = Backbone.Model.extend({

This comment has been minimized.

Copy link
@guerler

guerler Jul 29, 2017

Contributor

Is this model class needed? I think you can just use new Backbone.Model below.

'persistent': true,
'cls': 'errormessage'
};
self.display_message( self, error_response, self.$el.find( '.response-message' ) );

This comment has been minimized.

Copy link
@guerler

guerler Jul 29, 2017

Contributor

Parsing self as first parameter is not necessary. I made a similar comment in the other PR. Same goes for self.$el.find('') vs self.$('').

self.$el.empty().append( self._templateHeader() );
self.display_message( self, message, self.$el.find( '.response-message' ) );
// Create all tabs
self.create_tabs( self, response, self.$el.find( '.edit-attr' ) );

This comment has been minimized.

Copy link
@guerler

guerler Jul 29, 2017

Contributor

This should be just self.create_tabs( response ). The other items can be determined within the method.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Jul 31, 2017

It seems that the dataset id is not always correct. If I hover over the eye icon the correct id is displayed, while the edit button uses a different id. If that happens the form doesn't load.

@@ -60,12 +60,10 @@ var DatasetListItemEdit = _super.extend(
deleted = this.model.get( 'deleted' ),
editBtnData = {
title : _l( 'Edit attributes' ),
href : this.model.urls.edit,
target : this.linkTarget,
href : Galaxy.root + 'datasets/edit?dataset_id=' + this.model.attributes.dataset_id,

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Jul 31, 2017

Member

If you want to build the url this way the correct id is this.model.attributes.id
That would fix #4334 (comment)

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Jul 31, 2017

Really nice, if you can fix the dataset id mismatch I'm happy to merge.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Jul 31, 2017

@galaxybot test this

@mvdbeek mvdbeek merged commit bd0f804 into galaxyproject:dev Aug 1, 2017

5 checks passed

api test Build finished. 279 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 150 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 37 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@mvdbeek

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

Thanks a lot @anuprulez, this is much needed!

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2017

@mvdbeek thanks for merging :)

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.