Conversation
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughCSS refactors narrow selectors from FL-prefixed classes to custom .c-* classes, add Services component styles (including duplicates), introduce utility classes, migrate button styles, remove FL-specific layout/form/animation rules, delete a node-patterns stylesheet, drop several node width rules, and add Services classes to a services template. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Removed .fl-row from dual-class pseudo-element declarations - Keep only .c-row BEM classes for consistency - Tests pass, build clean
- Removed .fl-row:after from dual-class pseudo-element declarations - Keep only .c-row:after BEM class for consistency - FL-row dual-class cleanup completed for beaver-grid-layout.css
- Removed .fl-col from dual-class column declarations - Keep only .c-column BEM class for consistency - FL-column dual-class cleanup completed
- Removed .fl-photo from dual-class photo declarations - Keep only .c-photo BEM class for consistency - FL-photo dual-class cleanup completed
- Removed .fl-rich-text from dual-class typography declarations - Keep only .c-rich-text BEM class for consistency - FL-rich-text dual-class cleanup completed
- Added .c-services-hero class to hero section div alongside FL classes - Implemented shameless green CSS with critical FL-Builder styles - Added min-height: 100vh and display: flex for proper layout - Tests passing - visual regression resolved - Dual-class approach maintains FL compatibility
- Added .c-services-overview class to overview section div alongside FL classes - Implemented shameless green CSS with FL-Builder flexbox styles - Added min-height: 100vh and display: flex for consistency - Tests passing - dual-class approach working well - 2 of 5 priority sections now have BEM equivalents
- Add .c-services-clients class to client case studies section at line 178 - Apply shameless green FL-Builder replication with flexbox styles - Maintain visual parity with existing FL-Builder layout - Visual regression tests pass (0 failures)
- Add .c-services-cta class to final CTA section at line 260 - Apply shameless green FL-Builder replication pattern - Maintain visual parity with existing FL-Builder layout - Visual regression tests pass (0 failures) - Complete priority components migration for services template
- Step 1 of flocking rules: identify most alike patterns - Remove incomplete .c-button definition from components.css - Full definition remains in buttons-migration.css - Eliminates duplication between files
- Remove incomplete .c-button--large from components.css - Keep complete definition in buttons-migration.css - Eliminates padding conflict (32px vs 40px horizontal) - Single source of truth for large button styling
- Add .u-form-field-padding utility (padding: 0.75rem 1rem) - Add .u-form-field-transition utility for focus states - Consolidates 3 duplicate padding patterns - Consolidates 3 duplicate transition patterns - Prepared for field pattern replacement
- Add .u-gap-md utility (gap: 1rem) - Add .u-flex-gap utility (display: flex + gap: 1rem) - Consolidates 10+ duplicate gap: 1rem patterns - Consolidates 5+ duplicate flex + gap combinations - Ready for pattern replacement across components
* Add .u-text-body utility for font-size: 1rem + line-height: 1.5 * Consolidates 8+ duplicate instances found in forms.css * Common pattern used across form fields and controls
- Remove .fl-animation, .fl-animated classes (confirmed unused) - Eliminate 8 lines of animation-related CSS - First step in FL-Builder CSS cleanup to reduce file size
- Remove .fl-bg-video-fallback class (confirmed unused) - Eliminate 9 lines of background video CSS - Continue FL-Builder CSS cleanup
- Remove .fl-form-field, .fl-form-error, .fl-form-error-message, .fl-form-button-disabled - Eliminate 20 lines of form-related CSS (confirmed unused) - Continue FL-Builder CSS size reduction
- Removed single-occurrence FL node pattern from fl-use-cases-layout.css - Pattern only had width: 25% rule - Tests passed: 39 runs, 57 assertions, 0 failures - Part of Phase 3 FL-Builder cleanup
- Removed single-occurrence FL node pattern from fl-component-layout.css - Pattern only had width: 100% rule - Tests passed: 39 runs, 57 assertions, 0 failures - Part of Phase 3 FL-Builder cleanup
- Removed single-occurrence FL node pattern from fl-component-layout.css - Pattern only had width: 26% rule - Tests passed: 39 runs, 57 assertions, 0 failures - Part of Phase 3 FL-Builder cleanup
pftg
force-pushed
the
refact/components-fixes-3
branch
from
88b0781 to
6317806
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements a systematic refactoring to clean up CSS patterns and consolidate duplicate styles by introducing BEM-style components and utility classes. The changes focus on removing redundant FL-Builder-specific styles while maintaining functionality.
- Add new component classes (c-services-hero, c-services-overview, etc.) to HTML templates
- Remove unused CSS rules and consolidate duplicate patterns into utility classes
- Eliminate entire CSS files that contained redundant styles (fl-node-patterns.css)
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| themes/beaver/layouts/services/single.html | Adds BEM component classes to existing FL-Builder div elements |
| themes/beaver/assets/css/fl-use-cases-layout.css | Removes unused .fl-node-73fx8mcb9lte width rule |
| themes/beaver/assets/css/fl-service-detail-layout.css | Removes unused .fl-node-2il86phfbmex width rule |
| themes/beaver/assets/css/fl-node-patterns.css | Completely removes file containing form-specific FL-Builder styles |
| themes/beaver/assets/css/fl-homepage-layout.css | Consolidates .fl-col selector into existing .c-column rule |
| themes/beaver/assets/css/fl-component-layout.css | Removes multiple unused CSS rules and consolidates selectors |
| themes/beaver/assets/css/components/css-utilities.scss | Adds utility classes for form fields, layout gaps, and typography |
| themes/beaver/assets/css/components/c-typography.scss | Consolidates .fl-rich-text selector into existing .c-rich-text rule |
| themes/beaver/assets/css/components.css | Moves button styles to consolidation file and updates comments |
| themes/beaver/assets/css/component-bundle.css | Adds new services component styles with FL-Builder integration |
| themes/beaver/assets/css/beaver-grid-layout.css | Consolidates .fl-row selectors into existing .c-row rules |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
+2821
to
+2830
| .c-services-hero.fl-row-default-height .fl-row-content-wrap { | ||
| min-height: 100vh; | ||
| display: flex; | ||
| } | ||
|
|
||
| /* Services Hero - Pure BEM (Final Consolidation) */ | ||
| .c-services-hero .fl-row-content-wrap { | ||
| min-height: 100vh; | ||
| display: flex; | ||
| } |
There was a problem hiding this comment.
These two selectors have identical styles. The second selector (.c-services-hero .fl-row-content-wrap) is more general and would apply to elements matching the first selector as well. Consider removing the redundant first selector or clarify the distinction between them.
Comment on lines
+2833
to
+2842
| .c-services-overview.fl-row-default-height .fl-row-content-wrap { | ||
| min-height: 100vh; | ||
| display: flex; | ||
| } | ||
|
|
||
| /* Services Overview - Pure BEM (Final Consolidation) */ | ||
| .c-services-overview .fl-row-content-wrap { | ||
| min-height: 100vh; | ||
| display: flex; | ||
| } |
There was a problem hiding this comment.
Duplicate CSS rules with identical styles. The more general selector (.c-services-overview .fl-row-content-wrap) makes the first one redundant. Consider consolidating these into a single rule.
Comment on lines
+2845
to
+2854
| .c-services-clients.fl-row-default-height .fl-row-content-wrap { | ||
| min-height: 100vh; | ||
| display: flex; | ||
| } | ||
|
|
||
| /* Services Clients - Pure BEM (Final Consolidation) */ | ||
| .c-services-clients .fl-row-content-wrap { | ||
| min-height: 100vh; | ||
| display: flex; | ||
| } |
There was a problem hiding this comment.
Another instance of duplicate CSS rules. The pattern repeats across services components where the more specific selector is redundant when the general one exists.
Comment on lines
+2864
to
+2879
| .c-services-hero-standalone { | ||
| min-height: 100vh; | ||
| display: flex; | ||
| width: 100%; | ||
| } | ||
|
|
||
| .c-services-overview-standalone { | ||
| min-height: 100vh; | ||
| display: flex; | ||
| width: 100%; | ||
| } | ||
|
|
||
| .c-services-clients-standalone { | ||
| min-height: 100vh; | ||
| display: flex; | ||
| width: 100%; |
There was a problem hiding this comment.
These three standalone component classes have identical styles (min-height: 100vh, display: flex, width: 100%). Consider creating a shared utility class like .u-full-height-flex to reduce duplication.
Suggested change
| .c-services-hero-standalone { | |
| min-height: 100vh; | |
| display: flex; | |
| width: 100%; | |
| } | |
| .c-services-overview-standalone { | |
| min-height: 100vh; | |
| display: flex; | |
| width: 100%; | |
| } | |
| .c-services-clients-standalone { | |
| min-height: 100vh; | |
| display: flex; | |
| width: 100%; | |
| /* Utility class for full-height flex containers */ | |
| .u-full-height-flex { | |
| min-height: 100vh; | |
| display: flex; | |
| width: 100%; | |
| } | |
| /* Standalone BEM Components use the utility class for shared styles */ | |
| .c-services-hero-standalone, | |
| .c-services-overview-standalone, | |
| .c-services-clients-standalone { | |
| /* Add the utility class in markup, or use this selector for shared styles */ | |
| /* If additional component-specific styles are needed, add them below */ | |
| /* For now, all share the .u-full-height-flex styles */ |
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.
Summary by CodeRabbit
New Features
Style
Chores