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

Remove history renaming mako #4681

Merged
merged 9 commits into from Sep 26, 2017

Conversation

Projects
None yet
3 participants
@guerler
Contributor

guerler commented Sep 23, 2017

This PR removes the history rename form mako of the analysis app and builds the input form using the client-side builder instead. The methodolgy is the same we established for admin grid forms.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Sep 26, 2017

This breaks tests - I'm working on a fix.

return {
'title' : 'Change history name(s)',
'inputs' : [{
'name' : trans.security.encode_id(h.id),

This comment has been minimized.

@jmchilton

jmchilton Sep 26, 2017

Member

This seems like a significant regression in terms of... I don't know what the word is... I guess navigability of the DOM. The DOM is not as semantically useful as it was previously, I'll merge this but I really hope you consider these issues going forward. It sets a "tour_id" (an unfortunate name given that it has many applications besides tours including tests and other automated driving of the UI) but the ID isn't something that can be predicted for tours and it isn't something intuitive or stable across histories. There is no form element for the form (there should probably be one IMO - xref). Hitting enter no longer submits the form like it did before. To find the input itself I had to use '.ui-form-element input.ui-input' instead of the more intuitive input[name="name"] where it is clear what the input is for (the name).

screen shot 2017-09-26 at 9 09 28 am

I don't mind all the extra divs and such - as long as semantically intuitive selectors can be constructed.

This comment has been minimized.

@dannon

dannon Sep 26, 2017

Member

This is something we've argued about across several issues; I'll second (again) the concern here. "tour_id" should not be a necessary construct, when we already have a language for describing identifiers on the page. We just need to use it better.

This comment has been minimized.

@guerler

guerler Sep 26, 2017

Contributor

Thx for the review. I modified the input element names in #4700 by relying on enumeration. The resulting tour_ids are now name_INDEX instead of containing the encoded history id.

@jmchilton jmchilton merged commit 0dd7f01 into galaxyproject:dev Sep 26, 2017

0 of 6 checks passed

api test Test started.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
framework test Test started.
Details
integration test Test started.
Details
lgtm analysis: JavaScript Fetching Git Commits
Details
toolshed test Test started.
Details

mvdbeek added a commit to bardin-lab/galaxy that referenced this pull request Dec 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment