Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @LlGC-jop -- looks good overall, but I have one question below.
| const uvDiv = document.createElement("div"); | ||
| parent.appendChild(uvDiv); | ||
|
|
||
| const uv = new UniversalViewer({ |
There was a problem hiding this comment.
This is the only change here that might potentially change behavior. It looks like previously, we intentionally registered the resize handler before initializing the UV. I'm not sure why that was, or how significant it is. If this change is safe, then we should remove the if (uv) check inside the resize method below, since it should be safe to assume the variable is always defined... but is it possible there's some reason it's not safe? Did you do any investigation? Do you think we should look into this a little more deeply before merging?
There was a problem hiding this comment.
I had a similar concern after I moved that line, but it seems fine.
The resize handler only runs when the window triggers it or when the uv variable itself triggers it via an event.
I put a couple of console logs in and they fire when expected and uv is defined as expected.
I think we should keep the if() however, just in case, as I assume we don't want to make changes to parent's width/height if uv isn't defined.
I have however changed it to a guard clause because it makes the code a little cleaner.
There was a problem hiding this comment.
I don't think there's any harm in doing this, but I'm not sure if it's necessary -- if we assign const uv to an object prior to setting the event handler, I'm not sure how it's possible for uv to ever evaluate to a false-y value.
I also wonder, if a uv check is needed at all, if it would actually make more sense to put that around the uv.resize call instead of blocking the non-uv-related behavior.
In any case, happy to accept this as-is in the interest of getting this done... but I still have a feeling that this may be more complicated than necessary.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Giving this an approval so it can be merged -- but let's discuss at today's stand-up first.
2 participants
Resolves #1429.
Updates the eslint config to error when
letcould/should beconstRuns that config over the src directory
Manually changes
canvasanduvvars to be const because ESLint was unable to do it automatically.