Merged
Conversation
* Add image aspect ratio crops * Aspect ratio option * clean up * Update aspectRatioImage * use getImageSources for aspect ratio images * lint * Make tile card superheading the heading * mega menu card external link icon and a11y udpate * Remove unused import * Get rid of alt for most cards * Make gallery slide largest image src larger; clean up getImageSources and AspectRatioImage; no need to use srcset for campaign hero logo * Simplify AspectRatioImage * clean up * Add width and height for campaign header logo * more clean up * Add tabindex=-1 to skip link destinations per SODA recommendation
✅ Deploy Preview for adapt-giving ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements changes to improve image handling and accessibility features across the codebase. The main purpose is to refactor image processing, update responsive image breakpoints, and enhance accessibility for navigation and keyboard users.
- Refactored image processing to use aspect ratios and direct image sizing instead of complex responsive breakpoint systems
- Updated responsive breakpoints and simplified image source generation
- Enhanced accessibility with improved skip links and tabIndex management for keyboard navigation
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| utilities/getImageSources.ts | Simplified responsive image generation by removing custom breakpoints and original dimensions parameters |
| styles/tables.css | Added explicit white background for odd table rows |
| components/TileCard/* | Restructured component to use Heading for superheadline and removed alt prop handling |
| components/Storyblok/partials/PageLayout.tsx | Added tabIndex to main content for accessibility |
| components/Storyblok/SbStoryImage.tsx | Added aspect ratio support and image focus handling |
| components/Storyblok/SbInteriorPage.tsx | Enhanced skip link functionality with conditional targeting |
| components/Storyblok/SbCampaignPage/* | Improved image sizing and removed complex responsive image logic |
| components/Image/* | Major refactor to use aspect ratios with comprehensive breakpoint definitions |
| components/Media/MediaWrapper.tsx | Removed fixed height wrapper for full-width images |
| components/OverhangCard/OverhangCard.styles.ts | Fixed CSS selector syntax |
| components/EmbedVideo/EmbedVideo.tsx | Restructured video wrapper to use proper aspect ratio container |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| {!!getNumBloks(contactInfo) && ( | ||
| <div className="max-md:mb-45 md:max-lg:mb-72"> | ||
| {hasContactInfo && ( | ||
| <div className="max-md:mb-45 md:max-lg:mb-72" id="contact-info" tabIndex={hasContentMenu ? -1 : null}> |
There was a problem hiding this comment.
The tabIndex should be -1 or undefined, not null. Use undefined when the tabIndex should not be set.
| <div id="body-content" className={layout === 'left-sidebar' ? 'lg:col-span-8 xl:col-start-5' : 'lg:col-span-10 lg:col-start-2 xl:col-span-8 xl:col-start-3'}> | ||
| <div | ||
| id="body-content" | ||
| tabIndex={hasContentMenu && !hasContactInfo ? -1 : null} |
There was a problem hiding this comment.
The tabIndex should be -1 or undefined, not null. Use undefined when the tabIndex should not be set.
Suggested change
| tabIndex={hasContentMenu && !hasContactInfo ? -1 : null} | |
| tabIndex={hasContentMenu && !hasContactInfo ? -1 : undefined} |
| export const imageWrapper = 'shrink-0 grow-0 overflow-hidden -mt-80'; | ||
|
|
||
| export const image = (hasLink: boolean) => hasLink && 'group-hocus-within:scale-105 will-change transition-transform *:w-full'; | ||
| export const image = (hasLink: boolean) => hasLink && 'group-hocus-within:scale-105 will-change transition-transform w-full'; |
There was a problem hiding this comment.
The will-change property should specify which property will change, e.g., will-change-transform instead of just will-change.
Suggested change
| export const image = (hasLink: boolean) => hasLink && 'group-hocus-within:scale-105 will-change transition-transform w-full'; | |
| export const image = (hasLink: boolean) => hasLink && 'group-hocus-within:scale-105 will-change-transform transition-transform w-full'; |
Member
Author
|
Going to YOLO this one. Please review retro 🤠 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
READY FOR REVIEW
Summary
Review By (Date)
Criticality
Review Tasks
Setup tasks and/or behavior to test