fix: styles for uneven testimonial cards #2171
Conversation
Signed-off-by: Gitanshu Talwar <gitanshutalwar@gmail.com>
|
Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Newcomers' Guide and sure to join the community Slack. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @gitatractivo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini or gemini-code-assist here, providing a summary of this pull request to help everyone get up to speed quickly. This PR, titled "fix: styles for uneven testimonial cards", addresses issue #14861, which involves the testimonial cards in the footer slider having inconsistent heights, leading to an uneven and less visually appealing layout.
The changes implement a two-pronged approach to ensure the testimonial cards have equal heights. It combines a JavaScript solution that dynamically calculates and sets heights with CSS modifications using Flexbox to encourage elements to stretch and fill available space. The goal is to create a uniform appearance for the testimonial slider.
Highlights
- JavaScript Height Equalization: A new JavaScript function
equalizeTestimonialHeightsis added to_includes/footer.html. This function finds all testimonial quote elements within the slider, calculates the maximum height among them, and then sets all cards to that maximum height. This function is triggered on document ready, after various Slick slider events (init, reInit, afterChange, beforeChange, breakpoint), on window resize, and periodically via an interval to ensure heights remain consistent. - CSS Flexbox Styling: CSS rules in
_sass/testimonials.scssare updated to utilize Flexbox. Properties likedisplay: flex,flex-direction: column, andflex: 1are applied to various container and quote elements within the testimonial structure (.bq-section,.type-one-wrapper,.type-one-wrapper-boxed,.type-two-quote,.type-three-quote,.type-three-quote-base,.type-one-quote). This helps the elements stretch vertically to fill their containers, complementing the JavaScript approach and potentially offering a more native layout solution. - HTML Structure Updates: The class
common-wrapperis added to the.bq-sectionand.type-one-wrapperdivs in_includes/testimonials.html. This likely serves as a hook for the new CSS rules aimed at standardizing the layout and enabling the Flexbox behavior for height equalization. - Removed Fixed Heights: Fixed
min-heightvalues (specificallymin-height: 250px;) are removed from.type-one-wrapper.type-one-wrapper-boxedand.type-three-quotein_sass/testimonials.scss. This allows the elements to be sized dynamically by the Flexbox layout and the JavaScript height equalization logic.
Changelog
Click here to see the changelog
- _includes/footer.html
- Added a new JavaScript function
equalizeTestimonialHeights(lines 163-186) to calculate and set equal heights for testimonial cards. - Modified the Slick slider initialization to call
equalizeTestimonialHeightson various slider events (init,reInit,afterChange,beforeChange,breakpoint) (lines 266-268). - Added initial calls to
equalizeTestimonialHeightson document ready with timeouts (lines 271-272). - Added a window resize listener to re-equalize heights when the browser window is resized (lines 275-277).
- Added a recurring interval timer to call
equalizeTestimonialHeightsperiodically (lines 280). - Added
adaptiveHeight: falseto the Slick slider configuration (line 198).
- Added a new JavaScript function
- _includes/testimonials.html
- Added the
common-wrapperclass to the.bq-sectiondiv for testimonial type 1 (line 7). - Added the
common-wrapperclass to the.type-one-wrapper.type-one-wrapper-boxeddiv for testimonial type 1 (line 8). - Added the
common-wrapperclass to the.bq-sectiondiv for testimonial type 2 (line 26). - Added the
common-wrapperclass to the.type-one-wrapper.type-one-wrapper-boxeddiv for testimonial type 2 (line 27). - Added the
common-wrapperclass to the.bq-sectiondiv for testimonial type 3 (line 46). - Added the
common-wrapperclass to the.type-one-wrapper.type-one-wrapper-boxeddiv for testimonial type 3 (line 47).
- Added the
- _sass/testimonials.scss
- Removed a blank line (line 41).
- Removed
min-height: 250px;from.type-one-wrapper.type-one-wrapper-boxedand addedflex: 1;(lines 59, 59). - Added
display: flex; flex-direction: column; flex: 1;to.type-two-quote(lines 173-175). - Removed
min-height: 250px;from.type-three-quoteand addeddisplay: flex; flex-direction: column; flex: 1;(lines 270-272). - Added
flex: 1; display: flex; flex-direction: column;to.type-three-quote .type-three-quote-base(lines 283-285). - Added a new section of CSS rules using Flexbox to attempt CSS-only height equalization for various slider and testimonial elements (lines 354-403).
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
CSS and JS unite,
To make cards stand just right.
No more uneven view,
A layout fresh and new.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
✅ Deploy Preview for mesheryio-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Code Review
This pull request aims to fix uneven testimonial card heights by introducing JavaScript for height equalization and new CSS styles. The JavaScript approach involves multiple setTimeout calls and a setInterval, which raises concerns about performance and robustness. Additionally, a significant block of CSS labeled as an "Alternative CSS-only approach" has been added, and its relationship with the JavaScript solution needs clarification. The common-wrapper class added in the HTML also needs its purpose and styling defined or clarified if its styles are covered elsewhere.
Overall, the goal is clear, but the implementation strategy between JavaScript and CSS needs to be more streamlined and justified to ensure maintainability and performance. Good job on identifying the adaptiveHeight: false setting for the slick slider, which is crucial for this kind of manual height adjustment.
Summary of Findings
- JavaScript Height Equalization Strategy: The JavaScript in
_includes/footer.htmluses multiplesetTimeoutcalls with arbitrary delays and asetIntervalfor height equalization. This can lead to performance issues, brittleness, and complexity. The 10ms timeout inequalizeTestimonialHeightsmight be unreliable. - CSS vs. JavaScript for Equal Heights: A large CSS block in
_sass/testimonials.scss(lines 354-403) is labeled an "Alternative CSS-only approach". Its role and interaction with the JavaScript solution are unclear. If active, it might make some JS redundant or conflict. If it's an unused alternative, it should be removed or fully commented. - Missing/Redundant
common-wrapperCSS: Thecommon-wrapperclass is added in_includes/testimonials.htmlbut lacks explicit CSS definitions in the SCSS changes. If its intended styles are covered by direct styling of elements in the new CSS block, it might be redundant. - Use of
!importantin CSS: The "Alternative CSS-only approach" uses!important(e.g.,_sass/testimonials.scsslines 357, 363), which should generally be avoided. - JavaScript
varusage (Not commented due to severity settings): The JavaScript code usesvarfor variable declarations. Modern JavaScript prefersletandconstfor better scope management. - Debouncing Resize Handler (Implicitly covered, but to note): The window resize event handler in JavaScript could benefit from debouncing to improve performance.
Merge Readiness
The pull request addresses an important UI issue, but there are several high and medium severity concerns regarding the implementation strategy, particularly the interaction between the JavaScript and CSS solutions for equalizing card heights, the use of multiple timers in JavaScript, and the clarity of the common-wrapper class.
I recommend that these points, especially the high-severity ones, be addressed before merging to ensure a robust, maintainable, and performant solution. As a reviewer, I am not authorized to approve the pull request directly; further review and approval will be needed once changes are made.
| }).on('init reInit afterChange beforeChange breakpoint', function() { | ||
| // Equalize heights after slider events | ||
| setTimeout(equalizeTestimonialHeights, 150); | ||
| }); | ||
|
|
||
| // Initial height equalization | ||
| setTimeout(equalizeTestimonialHeights, 700); | ||
|
|
||
| // Re-equalize on window resize | ||
| $(window).on('resize', function() { | ||
| setTimeout(equalizeTestimonialHeights, 200); | ||
| }); | ||
|
|
||
| // Additional equalization on intervals to catch any missed updates | ||
| setInterval(equalizeTestimonialHeights, 3000); |
There was a problem hiding this comment.
The script employs multiple setTimeout calls with various delays (150ms, 700ms, 200ms) and a setInterval (3000ms) to trigger equalizeTestimonialHeights. This approach has a few potential drawbacks:
- Arbitrary Delays: The specific timeout values seem arbitrary. Are these based on specific observed needs, or trial and error? Such delays can be brittle and may not work reliably under all conditions (e.g., slower devices, network latency affecting image loads if card height depends on images).
- Performance of
setInterval: ThesetInterval(equalizeTestimonialHeights, 3000)will run the height equalization logic every 3 seconds, regardless of whether it's needed. This can lead to unnecessary CPU usage and potential layout thrashing, especially on pages left open for a long time. It often indicates that event-based triggers might not be catching all necessary updates. - Complexity: Managing multiple timers for the same function can make the logic harder to follow and debug.
Could the reliance on setInterval be reduced or eliminated by ensuring the event-based triggers (init, reInit, afterChange, etc.) are comprehensive? If setInterval is a must, what specific scenarios does it cover that event-based triggers do not? For the resize event, consider debouncing the equalizeTestimonialHeights call to prevent excessive firing during a resize storm.
| <div class="bq-section common-wrapper"> | ||
| <div class="type-one-wrapper type-one-wrapper-boxed common-wrapper"> |
There was a problem hiding this comment.
The common-wrapper class has been added to div.bq-section and div.type-one-wrapper.type-one-wrapper-boxed here (and similarly for other testimonial types). However, there are no corresponding CSS rules explicitly defined for .common-wrapper in the _sass/testimonials.scss changes.
While the new CSS block (lines 354-403 in _sass/testimonials.scss) does style .bq-section and .type-one-wrapper directly with flex properties, this makes the purpose of common-wrapper unclear:
- If
common-wrapperis intended to apply these flex/height styles, it might be redundant now that the elements are styled directly. - If
common-wrapperis intended for other common styles, those definitions are missing.
Could you clarify the role of common-wrapper and ensure its styles are defined if it's not redundant?
| // Alternative CSS-only approach for equal heights | ||
| .slider.slick-initialized { | ||
| .slick-track { | ||
| display: flex !important; | ||
| align-items: stretch; | ||
| } | ||
|
|
||
| .slick-slide { | ||
| height: auto; | ||
| display: flex !important; | ||
| flex-direction: column; | ||
|
|
||
| > div { | ||
| height: 100%; | ||
| display: flex; | ||
| flex-direction: column; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Ensure all testimonial wrappers stretch to full height | ||
| .bq-section { | ||
| height: 100%; | ||
| display: flex; | ||
| flex-direction: column; | ||
| } | ||
|
|
||
| .type-one-wrapper { | ||
| height: 100%; | ||
| display: flex; | ||
| flex-direction: column; | ||
| } | ||
|
|
||
| .type-one-wrapper-boxed { | ||
| height: 100%; | ||
| display: flex; | ||
| flex-direction: column; | ||
| } | ||
|
|
||
| // Make sure Type 2 and Type 3 quote containers stretch (preserve Type 1's row layout) | ||
| .type-two-quote, | ||
| .type-three-quote { | ||
| flex: 1; | ||
| } | ||
|
|
||
| // Type 1 should maintain its row layout, just stretch vertically | ||
| .type-one-quote { | ||
| flex: 1; | ||
| // Keep existing display: flex and flex-direction: row from original CSS | ||
| } |
There was a problem hiding this comment.
This large block of CSS is commented as an "Alternative CSS-only approach for equal heights". This raises a few important questions:
- Is this CSS active and intended to work alongside the JavaScript solution? The PR description mentions both CSS changes and adding a script.
- If it is active, how does it interact with the JavaScript
equalizeTestimonialHeightsfunction? Does the JS complement this CSS, or does one make the other redundant for certain aspects? - If this CSS-only approach is effective, could it simplify or replace parts of the JavaScript solution? CSS-based solutions for layout are generally preferred for performance.
- If this is truly an alternative and not part of the primary solution, should it be removed or fully commented out (e.g., using
/* ... */) to avoid confusion and potential unintended styles?
The use of !important (e.g., on lines 357 and 363) is also generally discouraged as it can make styles harder to manage and override. Are these !important flags strictly necessary for this approach to work?
Please clarify the strategy here. A clear, single source of truth for height equalization (or a well-defined interaction if both are needed) would be ideal.
| setTimeout(function() { | ||
| // Find the tallest card among ALL cards (including clones) | ||
| var maxHeight = 0; | ||
| $allCards.each(function() { | ||
| var cardHeight = $(this).outerHeight(); | ||
| if (cardHeight > maxHeight) { | ||
| maxHeight = cardHeight; | ||
| } | ||
| }); | ||
|
|
||
| // Set all cards to the same height | ||
| $allCards.css('height', maxHeight + 'px'); | ||
| }, 10); |
There was a problem hiding this comment.
The setTimeout with a 10ms delay to recalculate heights after resetting to 'auto' is quite aggressive. While deferring execution is a common pattern, such a short and fixed delay might not be universally reliable across different browsers or under heavy load, potentially leading to calculations based on an incompletely rendered state.
Could this be made more robust, perhaps by ensuring the reset has taken effect before proceeding, or is this delay empirically found to be sufficient in all tested scenarios? Consider if requestAnimationFrame could be a more suitable alternative for DOM-related timing if the goal is to sync with the browser's repaint cycle, though setTimeout(fn, 0) is also common for yielding.
|
Thanks for fixing the uneven sizing of the cards @gitatractivo |
|
these cards might be a little too big? what do you think? |
|
Yes, but the problem is height is dependent on the content. I can try to center the cards and reduce spacing around the cards. What do you think? |
|
@gitatractivo, you could try that, but in the end, it shouldn't look too off. feel free to start a discussion for suggestions in the websites channel. we can also discuss this in the upcoming sites meeting. |
Description
This PR fixes #14861
Notes for Reviewers
Signed commits