Add hotspot block that can support text, image and video #107
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
blocks/hotspot/hotspot.css
Outdated
| .block.hotspot > div.hotspot .hotspot-content video { | ||
| width: 400px; | ||
| height: auto; | ||
| max-width: unset !important |
blocks/hotspot/hotspot.css
Outdated
|
|
||
|
|
||
|
|
||
| .section.hotspot-container > .section-container { |
There was a problem hiding this comment.
This would mean that hotspot section would be styled differently and likely result in the style leaking over to other blocks in the same section. To avoid that, hotspot would need to be in its own section without anything else. Can we avoid this? One option would be to wrap the hostspot block in another div and apply these styles there to it without impacting container section styles.
blocks/hotspot/hotspot.css
Outdated
| } | ||
|
|
||
|
|
||
| main .embed .embed-default { |
There was a problem hiding this comment.
Why do we have styles for embed in hotspot block?
There was a problem hiding this comment.
not required, hence removed
blocks/hotspot/hotspot.css
Outdated
| } | ||
|
|
||
|
|
||
| .section.hotspot-container > .section-container > div:first-of-type { |
There was a problem hiding this comment.
Please see prior comment on similar lines.
blocks/hotspot/hotspot.css
Outdated
| } | ||
|
|
||
| @media (width >= 62rem) { | ||
| .section.hotspot-container > .section-container { |
There was a problem hiding this comment.
Avoid influencing section styles at block level.
blocks/hotspot/hotspot.css
Outdated
| margin: auto; | ||
| } | ||
|
|
||
| .section.hotspot-container > .section-container h1 { |
There was a problem hiding this comment.
Avoid influencing section styles at block level.
blocks/hotspot/hotspot.js
Outdated
| [...block.children].forEach((row, r) => { | ||
| if (r > 0) { | ||
| const content = [...row.children][0].textContent.trim(); | ||
| const isImage = content.endsWith('.jpg') || content.endsWith('.png') || content.endsWith('.gif') || content.endsWith('.jpeg'); |
There was a problem hiding this comment.
There could be images even without these extensions, for e.g. https://delivery-p66302-e574366.adobeaemcloud.com/adobe/assets/urn:aaid:aem:6928dc02-87be-4a00-93d5-7b3284f22e09
There was a problem hiding this comment.
Any image URL can be incorporated as per the requirement. We can use also use regex as per requirement which is upto customisation.
There was a problem hiding this comment.
yes but I am wondering if there's no way of telling the image by just looking at the URL. One option could be to express it as a variant of the block. For e.g. Hotspot(image) , Hotspot(video) etc. and based on that, decide the tag to inject.
There was a problem hiding this comment.
That will work but then wouldn't it limit the block to either have a specific type of hotspot content (either image or vide )and not a mix of contents.
There was a problem hiding this comment.
But yes, I will make the changes for fixed hotspot content.
blocks/hotspot/hotspot.js
Outdated
| if (r > 0) { | ||
| const content = [...row.children][0].textContent.trim(); | ||
| const isImage = content.endsWith('.jpg') || content.endsWith('.png') || content.endsWith('.gif') || content.endsWith('.jpeg'); | ||
| const isVideo = content.endsWith('.mp4') || content.endsWith('.webm') || content.endsWith('play') || content.endsWith('content/'); // Adjust condition as needed |
There was a problem hiding this comment.
Same as previous comment.
There was a problem hiding this comment.
Any video URL can be incorporated as per the requirement. We can use also use regex as per requirement which is upto customisation.
There was a problem hiding this comment.
yeah, bit I think there may be cases where just looking at the url may not tell. Maybe need a similar approach like here
blocks/hotspot/hotspot.js
Outdated
| const contentContainer = document.createElement('div'); | ||
| contentContainer.classList.add('hotspot-content'); | ||
|
|
||
| if (isImage) { |
There was a problem hiding this comment.
Maybe instead of explicitly adding markup for image, video etc, you could load a fragment and the fragment can contain anything. It'd be more generic. It may be a bit slow though.
There was a problem hiding this comment.
this is optional btw, and for now we can just support fixed hotspot content.
blocks/hotspot/hotspot.css
Outdated
| opacity: 1; | ||
| } | ||
|
|
||
| main .embed-default { |
There was a problem hiding this comment.
hostspot block should not influence embed-default styles. no?
blocks/hotspot/hotspot.js
Outdated
| const contentContainer = document.createElement('div'); | ||
| contentContainer.classList.add('hotspot-content'); | ||
|
|
||
| if (isImage) { |
There was a problem hiding this comment.
this is optional btw, and for now we can just support fixed hotspot content.
|
@sdmcraft Review comments incorporated. Also fixed hotspot variant added. |
This change adds a Hotspot block.
On an image, the user can place various hotspot at different positions that is configurable via top and left attribute in the doc.
The hotspot content supports text, image and video too which can be further extended as per the requirement. And all this while maintaining 100 LHS .
Fix #
Test URLs:
LHS is 100
pagespeed - https://pagespeed.web.dev/analysis/https-hotspotblock--franklin-assets-selector--hlxsites-hlx-live-hotspot-block-hotspot/imgjlh7yjk?form_factor=desktop