Career testimonials: create testimonials carousel block #71
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
|
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
The carousel buttons seem to be scrolling more than one item Screen.Recording.2023-09-27.at.6.33.06.PM.mov |
instead of scrolling to start and end
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
icons/angle-right-blue.svg
Outdated
There was a problem hiding this comment.
I think this will also impact the network-item block a bit due to the icon being shorter
Could you give me a heads up after you merge this so I can adjust the style accordingly? Thanks
Not sure about other blocks
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
There was a problem hiding this comment.
Screen.Recording.2023-09-28.at.13.14.19.mov
Your carousel can be controlled by swiping left-right on a touchpad
But it seems the "dots" at the bottom don't adjust, but I'm not sure what the desired behaviour is as the live one does not work with a touchpad (I assume it's the same for scrolling on a mouse but I don't use one in the office so I can't check)
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
| } | ||
| const la = document.createElement('img'); | ||
| la.src = '/icons/angle-left-blue.svg'; | ||
| la.alt = 'Previous person card'; |
There was a problem hiding this comment.
Yes we need i18n. I will create an issue to track.
|
|
||
| const ra = document.createElement('img'); | ||
| ra.src = '/icons/angle-right-blue.svg'; | ||
| ra.alt = 'Next person card'; |
There was a problem hiding this comment.
Yes we need i18n. I will create an issue to track.
|
LGTM. Please also add this to block library. |
| const link = document.createElement('button'); | ||
| link.textContent = 'Read More '; // TODO | ||
| const arrow = document.createElement('img'); | ||
| arrow.src = '/icons/angle-right-blue.svg'; |
There was a problem hiding this comment.
Why its needed here, can it be in css
There was a problem hiding this comment.
I guess it can be but then it has to be changed to a background image, these arrows arent really background images...
|
🔸 2 visual differences detected
The diff images are attached in the artifact |
|
Thanks all for the feedback! I created an issue for the i18n here: #80 |
Thanks @bosschaert Changes LGTM. |
Fixes #17
Test URLs:
I will add this block as well as the other career testimonial blocks to the block library separately once they are completely done.
Note that unit tests would be beneficial to this code. I will add them in a separate commit.