Skip to content

WIP inline slideshow #796

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

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

Conversation

zachgarwood
Copy link
Member

No description provided.

@zachgarwood zachgarwood self-assigned this Apr 30, 2025
Copy link

github-actions bot commented Apr 30, 2025

Automated PR Summary

This PR introduces a new feature for handling slideshow components on landing pages, specifically targeting a new type called "Research Center." The changes include updates across multiple files that add functionality and styling for the slideshow, integrate it within the landing page flow, and allow for its configuration in the backend. Additionally, the PR modifies existing CSS for better alignment with the new slideshow setup and extends the backend Twill CMS capabilities to manage these new types of content more effectively.

Potential bugs

  1. In the showcase.blade.php file, the logic for determining $mediaTypes might not handle cases other than 'Research Center' correctly due to commented-out lines which could potentially be an accidental change that affects other functionalities.
  2. The m-slideshow.blade.php view file has a typo in the class attribute (clas="m-slideshow__position") which should be class="m-slideshow__position". This typo could prevent the correct CSS styling from being applied to these elements.
  3. The navigation in the slideshow (both implementations) relies on id attributes formed by concatenating a loop index, which might result in non-unique IDs if multiple slideshows are present on the same page, leading to invalid HTML and potential JavaScript errors if IDs are used in scripts.

@zachgarwood zachgarwood marked this pull request as draft April 30, 2025 14:59
Copy link
Member Author

@zachgarwood zachgarwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are issues I'm seeing with both attempted implementations:

  1. This implementation is misaligned and improperly styled until you scroll to the final slide manually, then the whole component "rolls up" into a slideshow. Clicking on the slideshow position pips navigates to the correct slide the first or second time, but then stops working. The snapping to each slide in the slideshow while manually scrolling does work, tho!
  2. This implementation looks correct, but also clicking on the position pips only works the first time or so. Snapping to each slide in the slideshow on scroll does not work.

I believe the issue with clicking on the position pips is related to the focusDisplayHandler core behavior, because I see data-focus-method=mouse appear on the <a> when trying to navigate.

@@ -0,0 +1,246 @@
.m-slideshow.i-1 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the "Real Simple Slider" implementation: https://codepen.io/chriscoyier/pen/XwbNwX

}
}

.m-slideshow.i-2 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the "CSS-only Carousel Slider" implementation: https://codepen.io/Schepp/pen/WNbQByE

name='_implementation'
label='Implementation'
:required='true'
:options="['1', '2']"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just temporary


<div class="m-slideshow__carousel">
@foreach ($slides as $slide)
<div id="i-{{ $_implementation }}-slide-{{ $loop->iteration }}" class="m-slideshow__slide">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to figure out a good solution for the slide ids. Since there may be multiple slideshows on one page, each id has to be unique to both the slide and the block.

@zachgarwood zachgarwood force-pushed the feature/slideshow-carousel branch from f283248 to 7f1eddc Compare April 30, 2025 17:06
Copy link

sonarqubecloud bot commented May 5, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants