Hotspot.js: reduce complexity and add DOMpurify#672
Open
helms-charity wants to merge 3 commits intomainfrom
Open
Hotspot.js: reduce complexity and add DOMpurify#672helms-charity wants to merge 3 commits intomainfrom
helms-charity wants to merge 3 commits intomainfrom
Conversation
|
helms-charity
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DOMpurify related functions in scripts.js were already merged per scripts.js: Add global DOMpurify functions #673 but are repeated here. They are needed to sanitize document write methods.
applied the new sanitizeHTML to the places we were using innerHTML in hotspot.js
So content from DOM/dataset/contentMap is sanitized both when it’s first read/stored and again when it’s written to innerHTML, which addresses the aikido.dev finding.
Reduced nested functions for less complexity and better maintainability.
Summary of what was refactored with regard to nested functions:
runAfterDoubleRAF(fn)
Single helper that runs fn after two requestAnimationFrame calls, so the “double RAF then run” logic lives in one place.
schedulePadding
A small function that runs runAfterDoubleRAF(calculateInitialPadding), used both as the image load listener and in the “already loaded” path.
Removed waitForImageAndCalculate
Replaced with a direct if/else: if the image isn’t loaded yet, register schedulePadding on load; otherwise call runAfterDoubleRAF(calculateInitialPadding) immediately.
fadeTransition (was ~L300)
Introduced makeFadeInEndHandler() so the transition-end logic is no longer nested inside startFadeIn.
startFadeIn now only adds the class and attaches the handler returned by makeFadeInEndHandler().
Kept the double-RAF call as a one-liner: requestAnimationFrame(() => requestAnimationFrame(startFadeIn)).
Go-back link handling (was ~L748 and ~L947)
Added a shared handleGoBackLinkClick(evt, sectionData, container, reattachListeners) that:
Reads href and targetId. If no targetId: expands the hero and returns.
Otherwise loads content from sectionData.contentMap, runs fadeTransition, then calls the passed-in reattachListeners and showInitialPanel.
Both the initial decorate() setup and reattachHotspotListeners() now do:
link.addEventListener('click', (evt) => handleGoBackLinkClick(evt, sectionData, container, reattachHotspotListeners)).
Fix #671
Test URLs:
Testing:
There should be no difference in how The Concept animation works.
The low mobile score isn't related to this ticket.