ISSUE-31 update on window resize and path change #36
Conversation
app/public.js
Outdated
| */ | ||
| function refreshImagesInfo() { | ||
| hideImagesInfo(); | ||
| showImagesInfo(null); |
There was a problem hiding this comment.
@simonhearne DoD - why send null? there is no need. undefined is good enough
app/public.js
Outdated
| let path = window.location.pathname; | ||
| let update; | ||
|
|
||
| interval = setInterval(function() { |
There was a problem hiding this comment.
@simonhearne CONSISTENCY - setInterval(() => {})
app/public.js
Outdated
| let path = window.location.pathname; | ||
| let update; | ||
|
|
||
| interval = setInterval(function() { |
There was a problem hiding this comment.
@simonhearne DoD - should clear interval before creating a new one as we're always create another interval every time showImagesInfo is called
There was a problem hiding this comment.
It's cleared in hideImagesInfo(). Assume we never call showImagesInfo() twice without hideImagesInfo()? Then we would get duplicate overlays. Could add a check in showImagesInfo() for isImagesInforActive() then hide first... new issue for that?
There was a problem hiding this comment.
the idea is to not depend on a different method for clearing the interval. this couples those 2 methods together. in the case of calling showImagesInfo twice, it should be safe, including clearing existing overlays and existing intervals.
perhaps showImagesInfo should always call hideImagesInfo at the beginning to ensure a clean slate.
app/public.js
Outdated
| @@ -21,6 +21,8 @@ | |||
| const SVG_URL_REGEX = /\.svg/i; | |||
| const SVG_DATA_URI_REGEX = /^data:.*\/svg/i; | |||
| const MIN_IMAGE_SIZE = 10; | |||
| const CHECKER_INTERVAL_MS = 500; | |||
| let interval; | |||
There was a problem hiding this comment.
@simonhearne MAINTENANCE - use a more descriptive name like refreshImagesInterval
…checker into ISSUE-31-event_listeners
|
@simonhearne still need to add unit tests 😄 |
tabanliviu
changed the title
|
fixes #31 |
tabanliviu
changed the title
|
Verified ok |
app/public.js
Outdated
| /* istanbul ignore if */ | ||
| if (window.location.pathname !== path) { | ||
| update = true; | ||
| path = window.location.pathname; |
There was a problem hiding this comment.
@tabanliviu DoD - This doesn't include changes in the hash (either for traditional hash or non-HTML5 SPAs).
| /** | ||
| * Set event listeners | ||
| */ | ||
| function setRefreshImagesWatcher() { |
There was a problem hiding this comment.
@tabanliviu MAINTAINABILITY - Please clear the timeout in case another event implements the same method.
…ensure only one interval is created
|
@MalcolmSmithUK ready for review |
I ended up implementing an interval as one was required to check for path changes anyway, it also handily debounces the resize.
onHashChange doesn't always work e.g. https://www.oliverbonas.com/ is a SPA that updates the path, not hash. Set it at 500ms which seems like a reasonable compromise.