-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: global availability block #514
Conversation
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved because code looks good (at least visual result is great). But you should write a little bit of JS doc / comments because some parts look tricky and deserve some explanation. Think about someone else having to fix this code.
setTimeout(() => { | ||
if (svg.dataset.auto === 'true') unfocusPin(tooltip, svg); | ||
setTimeout(() => { | ||
iterateThroughPins(pins, svg, tooltip, randomIndex); // recursive call to focus another pin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates an infinite loop.
Writing some minimal doc to explain the method would help understand this is expected and desired.
blocks/availability/availability.js
Outdated
return randomIndex; | ||
} | ||
|
||
function tooltipAfterOverride(tooltip, axis, pixels) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method contains a lot of maths, but it is not obvious what it is doing or why it is needed...
blocks/availability/availability.js
Outdated
import { createTag } from '../../scripts/scripts.js'; | ||
|
||
function generateRandomNumber(max, excluding) { | ||
// minimum is inclusive, maximum is exclusive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No minimum
... ? What is excluding
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, much easier to understand now!
Description
more dynamic map of global availability
How Has This Been Tested?
https://avail-block--helix-website--adobe.aem.page/drafts/fkakatie/global
Types of changes
Checklist: