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

javascript router cleanup #5235

Merged
merged 1 commit into from Dec 20, 2017

Conversation

Projects
None yet
4 participants
@martenson
Member

martenson commented Dec 19, 2017

I think none of this code was being used or relevant.

}
},
authenticate: function(args, name) {

This comment has been minimized.

@dannon

dannon Dec 19, 2017

Member

This isn't dead code -- it's a default passthrough. The router is extended elsewhere, and authenticate is given functionality, which then inter-operates with the other methods here.

This comment has been minimized.

@martenson

martenson Dec 19, 2017

Member

Good point, do you know if this is meant as a client validation of requests (since we obviously have to have proper checks on the backend anyways)?

This comment has been minimized.

@dannon

dannon Dec 19, 2017

Member

Yes I do, and yes it is. It's faster and more user-friendly to validate this stuff on the client-side when we can.

This comment has been minimized.

@martenson

martenson Dec 20, 2017

Member

As a fan of thin clients I disagree here but will respect that.

This comment has been minimized.

@martenson

martenson Dec 20, 2017

Member

(I removed the second commit form the PR)

This comment has been minimized.

@dannon

dannon Dec 20, 2017

Member

Thanks for stripping the second commit. I think it's pretty widely accepted that clientside validation provides a significant UX improvement.

@dannon

This comment has been minimized.

Member

dannon commented Dec 20, 2017

To clarify for anyone else; $.isEmptyObject(data) will never be true here, so the conditional is pointless.

@dannon

This comment has been minimized.

Member

dannon commented Dec 20, 2017

@guerler You added the __identifier bits; is this only a requirement for routes with extra data in the get args, or do we always want it? (side note, I'd like to get rid of the random __identifiers when we can; it makes all the urls harder to parse at a glance)

@guerler

This comment has been minimized.

Contributor

guerler commented Dec 20, 2017

The __identifier is a workaround to enforce a page refresh. We need this because some links load a mako-based page into the iframe without changing the url. If the user then attempts to load that page with the previous url using the backbone router it will not trigger a page load. We made this addition in a follow-up without touching up the if statement following it.

@dannon

This comment has been minimized.

Member

dannon commented Dec 20, 2017

@guerler Got it; so to be safe we just add it everywhere for now, but we should be able to get rid of it eventually.

@dannon dannon merged commit 49f0cf8 into galaxyproject:dev Dec 20, 2017

6 checks passed

api test Build finished. 336 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 165 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 60 tests run, 0 skipped, 0 failed.
Details
selenium test Build finished. 117 tests run, 2 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details

@martenson martenson deleted the martenson:router-mods branch Dec 20, 2017

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