Reload functionality #682

2017-10-16T07:43:30+02:00

btzr-io commented

2017-10-16 07:43:30 +02:00

(Migrated from github.com)

Changes

Reload current page and clear the cache.
Add reload button inside header component.
Fixed issue with Published page after clearing the cache: https://github.com/lbryio/lbry-app/pull/682#issuecomment-337434699

### Changes - Reload current page and clear the cache. - Add reload button inside header component. - Fixed issue with `Published` page after clearing the cache: https://github.com/lbryio/lbry-app/pull/682#issuecomment-337434699 See: #672

kauffj (Migrated from github.com) reviewed 2017-10-16 07:43:30 +02:00

kauffj commented

2017-10-16 19:36:19 +02:00

(Migrated from github.com)

I kind of dislike adding this, because on one hand, this should never be necessary.

OTOH, it probably is currently, and there's this argument: https://www.tbray.org/ongoing/When/201x/2017/09/27/Refresh-Button

If we're going to give this such prominent placement, should it perhaps clear the cache as well? If a user wants to reload, it's likely because they are concerned about state being stale, and the application cache is a potential source of that.

I kind of dislike adding this, because on one hand, this should never be necessary. OTOH, it probably is currently, and there's this argument: https://www.tbray.org/ongoing/When/201x/2017/09/27/Refresh-Button If we're going to give this such prominent placement, should it perhaps clear the cache as well? If a user wants to reload, it's likely because they are concerned about state being stale, and the application cache is a potential source of that.

👍 1

kauffj commented

2017-10-17 21:52:54 +02:00

(Migrated from github.com)

@btzr-io any reason you closed this? My comment wasn't a no :)

👍 1

btzr-io commented

2017-10-18 02:35:16 +02:00

(Migrated from github.com)

This should never be necessary.

if shouldn't be necessary then why we add it? 😕 ^^

should it perhaps clear the cache as well?

Would this clear the history too?
it should reload the current page and don't take you somewhere else...

So I think the only real use will be for testing / debugging...

> This should never be necessary. if shouldn't be necessary then why we add it? :confused: ^^ > should it perhaps clear the cache as well? Would this clear the history too? it should reload the current page and don't take you somewhere else... So I think the only real use will be for testing / debugging...

kauffj commented

2017-10-18 03:27:09 +02:00

(Migrated from github.com)

if shouldn't be necessary then why we add it? 😕 ^^

Ideally it is not necessary. Pragmatically it may be. Check out the link, it's a solid argument.

I'm not 100% sure on cache clear behavior, but I agree that if it's clearing the history it's a bad idea.

> if shouldn't be necessary then why we add it? :confused: ^^ _Ideally_ it is not necessary. Pragmatically it may be. Check out the link, it's a solid argument. I'm not 100% sure on cache clear behavior, but I agree that if it's clearing the history it's a bad idea.

👍 1

btzr-io commented

2017-10-18 03:34:49 +02:00

(Migrated from github.com)

Also after clearing the cache, the published page shows:

It looks like you haven't published anything to LBRY yet.

This could cause some confusion since there is no indication of fetching / loading any data and looks like all your claims are gone 😛

Also after clearing the cache, the `published` page shows: > It looks like you haven't published anything to LBRY yet. This could cause some confusion since there is no indication of fetching / loading any data and looks like all your claims are gone :stuck_out_tongue:

kauffj commented

2017-10-18 03:41:22 +02:00

(Migrated from github.com)

If you also invoke the actions in this function I bet state is properly restored: b8980c00d3/ui/js/actions/app.js (L177)

If you also invoke the actions in this function I bet state is properly restored: https://github.com/lbryio/lbry-app/blob/b8980c00d363ea2fffc160ed9471c20bb4079127/ui/js/actions/app.js#L177

👍 1

kauffj commented

2017-10-18 03:41:37 +02:00

(Migrated from github.com)

(which ought to be refactored if invoked by refresh)

btzr-io commented

2017-10-18 05:47:49 +02:00

(Migrated from github.com)

Note UX

Would be nice to have an indication if the page it's reloadign, right now looks like if the app crashed.
( empty green screen )

### Note UX Would be nice to have an indication if the page it's reloadign, right now looks like if the app crashed. ( empty green screen )

btzr-io commented

2017-10-18 05:51:59 +02:00

(Migrated from github.com)

I guess we only need -> dispatch(doFetchFileInfosAndPublishedClaims());

I guess we only need -> ` dispatch(doFetchFileInfosAndPublishedClaims());`

btzr-io commented

2017-10-18 05:53:48 +02:00

(Migrated from github.com)

@kauffj yeah it works 😃 ^

@kauffj yeah it works :smiley: ^

btzr-io commented

2017-10-18 07:07:14 +02:00

(Migrated from github.com)

I was wrong history is fine,
Anyway do we need this?
782fb5d59e/ui/js/page/settings/view.jsx (L26)

I was wrong history is fine, Anyway do we need this? https://github.com/lbryio/lbry-app/blob/782fb5d59ebbe6934f8ec97b9f2e77653cec2cb4/ui/js/page/settings/view.jsx#L26

btzr-io commented

2017-10-18 07:47:58 +02:00

(Migrated from github.com)

I'm not 100% sure on cache clear behavior

I added it in the reload function, didn't found any issues...
the standard behavior of the refresh button it's just "reload current page" see any browser for reference...

it's likely because they are concerned about state being stale, and the application cache is a potential source of that.

but then I guess is ok to add it ^

> I'm not 100% sure on cache clear behavior I added it in the reload function, didn't found any issues... the standard behavior of the refresh button it's just "reload current page" see any browser for reference... > it's likely because they are concerned about state being stale, and the application cache is a potential source of that. but then I guess is ok to add it ^

btzr-io (Migrated from github.com) reviewed 2017-10-18 08:07:34 +02:00

ui/js/actions/app.js

btzr-io (Migrated from github.com) commented

2017-10-18 08:07:34 +02:00

Should I use the electron api or just window.location.reload ?

Should I use the electron api or just `window.location.reload` ?

btzr-io (Migrated from github.com) reviewed 2017-10-18 08:11:39 +02:00

ui/js/actions/app.js

					
				@ -202,3 +202,3 @@

				    window.cacheStore.purge();

				    dispatch(doFetchFileInfosAndPublishedClaims());

				    return Promise.resolve();

btzr-io (Migrated from github.com) commented

2017-10-18 08:11:39 +02:00

This fixed the issue with published page ^
See https://github.com/lbryio/lbry-app/pull/682#issuecomment-337434699

This fixed the issue with `published` page ^ See https://github.com/lbryio/lbry-app/pull/682#issuecomment-337434699

btzr-io (Migrated from github.com) reviewed 2017-10-19 20:32:22 +02:00

ui/js/actions/app.js

					
				@ -202,3 +202,3 @@

				    window.cacheStore.purge();

				    dispatch(doFetchFileInfosAndPublishedClaims());

				    return Promise.resolve();

btzr-io (Migrated from github.com) commented

2017-10-19 20:32:22 +02:00

weird, the issue is back ^ 😕

weird, the issue is back ^ :confused:

btzr-io commented

2017-10-20 02:39:57 +02:00

(Migrated from github.com)

This should be fixed before adding this...
see: https://github.com/lbryio/lbry-app/pull/682#issuecomment-337452542 and https://github.com/lbryio/lbry-app/pull/682#issuecomment-337434699

This should be fixed before adding this... see: https://github.com/lbryio/lbry-app/pull/682#issuecomment-337452542 and https://github.com/lbryio/lbry-app/pull/682#issuecomment-337434699

btzr-io commented

2017-10-20 02:41:17 +02:00

(Migrated from github.com)

Also https://github.com/lbryio/lbry-app/pull/688 make this unnecessary ( ? )