DeBraid / puck

Angular.JS Front-End code for Puckalytics.com
http://puckalytics.com/puck-master/
4 stars 1 forks source link

Copy Table URL to Clipboard is missing part of the path #3

Closed JesseDahl closed 8 years ago

JesseDahl commented 8 years ago

here's the url in the browser http://puckalytics.com/puck-master/#/goalies?team=&player_name=&position=&season=201516&situation=5v5&GPMin=0&GPMax=999&GAMin=0&GAMax=999&SAMin=0&SAMax=9999&FAMin=0&FAMax=9999&CAMin=0&CAMax=9999&GA60Min=0&GA60Max=100&SA60Min=0&SA60Max=100&FA60Min=0&FA60Max=100&CA60Min=0&CA60Max=100&SvPctMin=0&SvPctMax=100&FSvPctMin=0&FSvPctMax=100&CSvPctMin=0&CSvPctMax=100&TOIMin=100&TOIMax=99999

here's the url after clicking the button http://puckalytics.com/#/goalies?team=&player_name=&position=&season=201516&situation=5v5&GPMin=0&GPMax=999&GAMin=0&GAMax=999&SAMin=0&SAMax=9999&FAMin=0&FAMax=9999&CAMin=0&CAMax=9999&GA60Min=0&GA60Max=100&SA60Min=0&SA60Max=100&FA60Min=0&FA60Max=100&CA60Min=0&CA60Max=100&SvPctMin=0&SvPctMax=100&FSvPctMin=0&FSvPctMax=100&CSvPctMin=0&CSvPctMax=100&TOIMin=100&TOIMax=99999

You are missing the "puck-master" part of the url, near the beginning.

DeBraid commented 8 years ago

Thanks @JesseDahl ! I hacked something together here for a temp. fix. https://github.com/DeBraid/puck/commit/1e2628a1fab77c3cac1132bd9a58e66d491998ae

Once this project is polished and is migrated to apex domain of puckalytics.com will have to remember to remove this!

JesseDahl commented 8 years ago

Does $location.url() give you the full current url?

Maybe try that? Then you don't need to remember to undo anything. I think this will work but I'm just guessing, I don't have your app in front of me yet (I might someday! kinda busy rn)

JesseDahl commented 8 years ago

oh, i just read to the bottom of this http://stackoverflow.com/questions/20645783/how-can-i-get-the-full-url-of-the-current-ui-router-state

you might just have to use window.location or window.location.hash

hopefully that gives you what you need.

David-W-Johnson commented 8 years ago

I uploaded the changes to the website but it didn't seem to fix the problem.

DeBraid commented 8 years ago

@JesseDahl good call, $location.path() is more robust than hardcoding a string

@David-W-Johnson Per this commit, there should be a console.log message which I am not seeing in production. Perhaps pull form master any try to re-upload? https://github.com/DeBraid/puck/commit/87dae98297db70b0327ea07139bf508c51f3664e#diff-c23e425139419fc29c35244d493758edR35

JesseDahl commented 8 years ago

$location.path() won't include the url params (anything after the ?), but $location.url() will.

DeBraid commented 8 years ago

Params from existing URL are stale, given they are parsed from defaults. The purpose of this function is to extract updated values from the filters and then put them into a new URL to share.

The updated params come in via args below in copy-url directive.

function buildUrlToCopy (event, args) {
    var new_url = $state.href($state.current.name, args, {absolute: true});
    // FIXME DB - temporary hack the URL whilst in development
    var split_url = new_url.split('#');
    var hacked_URL = split_url[0] + 'puck-master/#' + split_url[1]; 
    console.log('hacked_URL, split_url', hacked_URL, split_url);
    scope.textToCopy = hacked_URL;
}
JesseDahl commented 8 years ago

ah, i see. In that case I would probably suggest updating the query parameters anytime a filter is applied, so that the url always reflects where you are in the app. i think $location.search() is what you want to use for that to prevent controller reload, maybe even chaining a call to replace() onto the end of it if you don't want every filter change added to the browsing history. I kinda lean towards adding it to the history since it only updates when you click the button instead of when you modify each filter input.

I think the URL is pretty important for this type of app, so it's probably best to have it reflect the current state of the filters. I think those filters are pretty central to the purpose of this tool, enough to merit URL changes.

David-W-Johnson commented 8 years ago

I did some testing and found out a few things about Url copying.

  1. buildUrlToCopy does not get called on page load so the default URL doesn't include the puck-master part. We should call buildUrlToCopy on page load or edit the default URL.
  2. The URL correctly gets updated if you reload data (i.e. load 5v5close) but if you then subsequently change a filter (i.e. the GP filter) and copy the URL it will revert back to 5v5 when it should remain at 5v5close.
David-W-Johnson commented 8 years ago

Forgot #3.

  1. The sort column and sort direction should also get included in the URL. This is an important feature I think.
DeBraid commented 8 years ago

Appears to be working as desired.

JesseDahl commented 8 years ago

As a user, I still think the browser's address should change when making filter or sort changes. You're going to get a lot of people sharing the wrong link if you don't, I think. Keep it in mind maybe. Just my opinion.

David-W-Johnson commented 8 years ago

I agree.

David

Quoting Jesse Dahl notifications@github.com:

As a user, I still think the browser's address should change when
making filter or sort changes. You're going to get a lot of people
sharing the wrong link if you don't, I think. Keep it in mind maybe.
Just my opinion.


Reply to this email directly or view it on GitHub: https://github.com/DeBraid/puck/issues/3#issuecomment-185854116