[UI] Make menu styles like on design #153
Conversation
pftg
changed the title
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request implements various changes across multiple HTML and CSS files within the Beaver theme. Key modifications include updates to CSS styles for navigation components, enhancements to image rendering templates, and adjustments to permalink structures for blog posts and job listings. These changes aim to improve layout, interaction areas, and maintainability without altering core functionality. Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
themes/beaver/assets/css/navigation.css (2)
405-415: LGTM! Enhanced touch target for arrowThe added pseudo-element creates a larger touch area (30x30px) around the arrow, improving mobile usability significantly.
Consider adding a comment to document the touch target size:
/* 30x30px touch target area for better mobile interaction */ .navigation .item .arrow:before {
Line range hint
1-434: Consider adding CSS custom properties for design tokensWhile the changes look good, consider introducing CSS custom properties (variables) for commonly used values like colors, spacing, and breakpoints. This would make future design updates easier to maintain and ensure consistency.
Example:
:root { --color-primary: #1a8cff; --color-text: #121212; --spacing-touch-target: 30px; --font-size-mobile: 18px; /* etc. */ }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
themes/beaver/assets/css/navigation.css(6 hunks)
🔇 Additional comments (6)
themes/beaver/assets/css/navigation.css (6)
3-3: Verify 11px vertical padding against design specs
The 1px padding adjustment seems very subtle. Could you confirm this specific measurement (11px) matches the Adobe XD design specs exactly?
93-94: LGTM! Improved touch target size
The increased dimensions (30x30px) provide a better touch target for mobile users, meeting accessibility guidelines.
184-184: LGTM! Clear active state indication
The active state styling using the primary blue color provides good visual feedback for the current selection.
340-340: LGTM! Improved mobile button spacing
The increased margin provides better vertical separation in the mobile menu layout.
425-430: LGTM! Appropriate mobile hover handling
Good practice to disable hover effects on mobile devices where they're not applicable.
431-434: LGTM! Enhanced mobile typography
The increased font size (18px) improves readability on mobile devices, and the right padding creates proper spacing for the arrow.
Please confirm these specific values match the Adobe XD design specs:
- Font size: 18px
- Right padding: 15px
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
test/mobile_site_test.rb (1)
32-32: Consider documenting the stability parameters.The addition of wait and stability parameters is good for test reliability. However, these specific values might need explanation for future maintainers.
Add a comment explaining the chosen values:
+ # Wait 3s for dynamic content, reduced stability time for performance assert_matches_screenshot "blog/index/_pagination", skip_area: [".blog-post"], wait: 3, stability_time_limit: 0.25
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
test/fixtures/screenshots/macos/mobile/nav/hamburger_menu.pngis excluded by!**/*.pngtest/fixtures/screenshots/macos/mobile/nav/hamburger_menu/services.pngis excluded by!**/*.png
📒 Files selected for processing (2)
test/desktop_site_test.rb(2 hunks)test/mobile_site_test.rb(1 hunks)
🔇 Additional comments (2)
test/desktop_site_test.rb (2)
37-37: LGTM! Good improvement to screenshot assertions.
Adding skip_area: [".blog-post"] helps focus the visual regression tests on navigation/menu changes by excluding dynamic blog content. This aligns well with the PR's objective of updating menu styles.
Also applies to: 45-45
180-182: Verify consistent usage of the new navigation selector.
The change from nav to .navigation selector is good for specificity, but let's verify it's used consistently across the codebase.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Navigation selector usage is consistent across the codebase
The codebase shows consistent usage of .navigation class:
- Main navigation structure is properly defined in
themes/beaver/assets/css/navigation.csswith.navigationclass - Navigation template uses
<nav class="navigation js-navigation">inthemes/beaver/layouts/partials/page/navigation.html - Test file correctly uses
.navigationselector matching the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of navigation selectors
# Test 1: Look for any remaining 'nav' tags that might need updating
echo "Checking for nav elements that might need updating:"
rg --type html --type erb "nav\s|<nav>|<nav\s" -A 2
# Test 2: Verify the new .navigation class usage
echo -e "\nChecking .navigation class usage:"
rg "\.navigation" -t css -t html -t erb -A 2
Length of output: 60284
There was a problem hiding this comment.
Menu still looks broken: https://www.loom.com/share/acc26aad5e674d1ca3041328c2c7ff2f
and mobile menu does not work anymore
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
themes/beaver/layouts/partials/blog/img-cropped.html (1)
Line range hint
1-16: Consider adding width and height attributes to source elements.While the changes look good, you might want to consider adding width and height attributes to the source elements as well, not just the img tag. This can help prevent layout shifts during page load.
- <source srcset="{{ $webp.RelPermalink }}" type="image/webp"> - <source srcset="{{ $cropped.RelPermalink }}" type="{{ $cropped.MediaType }}"> + <source srcset="{{ $webp.RelPermalink }}" type="image/webp" width="{{ .width }}" height="{{ .height }}"> + <source srcset="{{ $cropped.RelPermalink }}" type="{{ $cropped.MediaType }}" width="{{ .width }}" height="{{ .height }}">.dev/Dockerfile (1)
46-47: Document YJIT usage and port configuration.The YJIT (Yet Another JIT) compiler for Ruby is enabled, which is good for performance. However, consider adding a comment explaining this optimization choice for future maintainers.
+# Enable YJIT compiler for improved Ruby performance RUBY_YJIT_ENABLE=1 \ +# Port for running test server, ensure this doesn't conflict with other services TEST_SERVER_PORT=1314themes/beaver/layouts/_default/_markup/render-image.html (1)
Line range hint
7-8: Consider adding AVIF format supportThe current implementation supports webp and jpeg formats. Consider adding AVIF format support for better compression and quality:
-{{ $formats := slice "webp" "jpeg" }} +{{ $formats := slice "avif" "webp" "jpeg" }}This would provide better image quality and smaller file sizes for browsers that support AVIF.
themes/beaver/layouts/_default/list.html (1)
57-57: Consider adding 'noreferrer' to external links.While
rel="noopener"is good, addingnoreferrerprovides additional security by preventing the referrer header from being sent.-<a class="link" target="_blank" rel="noopener" href="{{ .RelPermalink }}"> +<a class="link" target="_blank" rel="noopener noreferrer" href="{{ .RelPermalink }}">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (13)
Gemfile.lockis excluded by!**/*.locktest/fixtures/screenshots/linux/desktop/about_us.pngis excluded by!**/*.pngtest/fixtures/screenshots/linux/desktop/blog/index.pngis excluded by!**/*.pngtest/fixtures/screenshots/linux/desktop/blog/index/_pagination.pngis excluded by!**/*.pngtest/fixtures/screenshots/linux/desktop/blog/post.pngis excluded by!**/*.pngtest/fixtures/screenshots/linux/desktop/careers.pngis excluded by!**/*.pngtest/fixtures/screenshots/linux/desktop/clients.pngis excluded by!**/*.pngtest/fixtures/screenshots/linux/desktop/clients/_footer.pngis excluded by!**/*.pngtest/fixtures/screenshots/linux/desktop/homepage/_footer.pngis excluded by!**/*.pngtest/fixtures/screenshots/linux/desktop/nav/services.pngis excluded by!**/*.pngtest/fixtures/screenshots/linux/desktop/nav/use_cases.pngis excluded by!**/*.pngtest/fixtures/screenshots/linux/mobile/free_consultation.pngis excluded by!**/*.pngtest/fixtures/screenshots/linux/mobile/nav/hamburger_menu.pngis excluded by!**/*.png
📒 Files selected for processing (12)
.dev/Dockerfile(1 hunks).dev/compose.yml(1 hunks)bin/build(1 hunks)bin/docked(1 hunks)bin/dtest(1 hunks)test/support/setup_capybara.rb(1 hunks)themes/beaver/layouts/_default/_markup/render-image.html(1 hunks)themes/beaver/layouts/_default/list.html(1 hunks)themes/beaver/layouts/blog/list.html(1 hunks)themes/beaver/layouts/page/careers.html(1 hunks)themes/beaver/layouts/partials/blog/img-cropped.html(1 hunks)themes/beaver/layouts/partials/clients/grid-single.html(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- bin/dtest
🔇 Additional comments (19)
bin/docked (1)
5-5: LGTM! Consider documenting the new behavior.
The addition of --quiet-pull and --remove-orphans flags follows Docker best practices. The --remove-orphans flag helps prevent resource leaks, while --quiet-pull reduces log verbosity.
Consider adding a comment above the command to document that:
- Pull progress output is suppressed
- Orphaned containers will be automatically removed
Let's verify the impact on CI/CD pipelines:
✅ Verification successful
LGTM! Consider documenting the new behavior in bin/docked
The changes to bin/docked are safe and follow best practices. From the verification:
- The script is only used by
bin/dtest, limiting the impact scope - It properly inherits error handling from
bin/composeviaset -eo pipefail - The flags are passed through to
docker composevia the wrapper chain
Consider adding a comment to document that:
# Runs docker compose with quiet pull output and automatic orphan container cleanup🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if this script is used in CI/CD workflows
# Expected: Find references in CI/CD configuration files
# Look for CI/CD config files that might use this script
fd -e yml -e yaml '(circle|github|gitlab|jenkins|azure)' .
# Check for direct usage of bin/docked in any YAML files
rg 'bin/docked' -t yaml
Length of output: 84
Script:
#!/bin/bash
# Let's try a broader search to understand usage patterns:
# 1. Find all workflow/CI files regardless of extension
fd 'workflow|ci|test|build' .github/
# 2. Search for any references to docker-compose or similar commands
rg -l "docker[- ]compose|bin/docked"
# 3. Look for shell scripts that might use this command
fd -t f -x file {} \; | grep "shell script"
Length of output: 62672
Script:
#!/bin/bash
# Let's try a simpler approach to find usage patterns:
# 1. Look for direct references to bin/docked
rg -l "bin/docked"
# 2. Check content of bin/docked and bin/compose
cat bin/docked
cat bin/compose
# 3. Look for docker-compose.yml files that might be affected
fd -e yml -e yaml 'docker-compose'
Length of output: 487
bin/build (3)
1-2: LGTM! Proper shebang line used.
The shebang line correctly uses /usr/bin/env bash which provides better portability across different systems.
5-5: 🛠️ Refactor suggestion
Verify Hugo installation and add error handling.
The script assumes Hugo is installed and available in PATH. Consider adding validation and providing a helpful error message if Hugo is not found.
#!/bin/bash
# Check if Hugo is mentioned in documentation
rg -l 'hugo' README.md || echo "No Hugo setup instructions found in README.md"
# Look for package management files that might install Hugo
fd -t f "package.json|Gemfile|requirements.txt|Dockerfile" Add this validation before running Hugo:
+if ! command -v hugo >/dev/null 2>&1; then
+ echo "Error: Hugo is not installed. Please install Hugo first." >&2
+ exit 1
+fi
+
hugo --gc --minify --environment production --forceSyncStatic --cleanDestinationDir --logLevel warn "$@"1-5: Ensure script has executable permissions.
The script needs to be executable to function properly.
Run this command to add executable permissions:
chmod +x bin/build.dev/compose.yml (2)
5-6: Clarify the relationship between Docker changes and UI menu styles.
These Docker configuration changes appear unrelated to the PR's stated objective of updating menu styles. Consider submitting these development environment improvements as a separate PR to maintain clear change scope and easier review process.
Let's check if there are any UI-related changes in this PR:
#!/bin/bash
# Look for CSS/SCSS files changes that might be related to menu styles
rg -l "menu|nav" --type=css --type=scss5-6: LGTM! Verify the new build paths.
The reorganization of Docker files looks good. The build context and Dockerfile paths have been updated to accommodate the new structure.
Let's verify the paths exist:
✅ Verification successful
✓ Docker configuration paths are valid and correctly structured
The verification confirms that all required files exist and are properly placed:
.dev/Dockerfileexists and is accessibleGemfileandGemfile.lockare present in the root directory- The
.devdirectory is properly organized with related Docker files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the Dockerfile and build context
if [ -f ".dev/Dockerfile" ]; then
echo "✓ .dev/Dockerfile exists"
else
echo "✗ .dev/Dockerfile not found"
fi
# Check for essential files in the parent directory (build context)
for file in Gemfile Gemfile.lock; do
if [ -f "../$file" ]; then
echo "✓ ../$file exists"
else
echo "✗ ../$file not found"
fi
done
Length of output: 70
Script:
#!/bin/bash
# Check if the Dockerfile exists in the .dev directory
ls -la .dev/Dockerfile 2>/dev/null || echo "Dockerfile not found in .dev directory"
# List contents of .dev directory to verify its existence and contents
ls -la .dev/ 2>/dev/null || echo ".dev directory not found"
# Check the repository root for build context files
ls -la Gemfile Gemfile.lock 2>/dev/null || echo "Gemfile(s) not found in root"
Length of output: 553
themes/beaver/layouts/partials/blog/img-cropped.html (2)
8-10: LGTM! Good improvement in URL handling for the picture element.
The switch to .RelPermalink for image sources is a positive change that improves portability across different environments and aligns with Hugo's best practices.
14-14: LGTM! Consistent URL handling for the placeholder image.
The change maintains consistency with the other image URL changes and ensures proper functioning in all environments.
.dev/Dockerfile (2)
49-51:
Improve Dockerfile best practices for copying Gemfiles.
The current COPY command using relative paths (../) is not recommended in Dockerfiles as it can lead to build context issues. Instead, adjust the build context in your docker-compose.yml and use local paths.
Apply this change:
-COPY ../Gemfile ../Gemfile.lock ./
+COPY Gemfile Gemfile.lock ./Also, consider optimizing the bundle install:
-RUN bundle check || bundle install
+RUN bundle config set --local without 'development test' && \
+ bundle install --jobs 4 --retry 3Let's verify the Gemfile location and compose configuration:
53-53: 🛠️ Refactor suggestion
Consider making the test command more flexible.
The hardcoded CMD ["bin/test"] might be too restrictive. Consider allowing override through environment variables or docker-compose configuration.
-CMD ["bin/test"]
+CMD ["sh", "-c", "${TEST_COMMAND:-bin/test}"]Likely invalid or redundant comment.
themes/beaver/layouts/_default/_markup/render-image.html (3)
31-31: Good addition of rel="nofollow"!
Adding rel="nofollow" to the image link is a good SEO practice as it prevents search engines from crawling these internal resource links.
42-46: Excellent implementation of performance best practices!
The image implementation includes:
- Lazy loading for better performance
- Explicit width/height to prevent layout shifts (CLS)
- Proper alt text for accessibility
39-40: Verify breakpoint consistency with design
The sizes attribute uses a 48rem breakpoint. Please verify this matches the menu breakpoints in the Adobe XD design.
✅ Verification successful
Based on the search results, I can see that 768px is consistently used as a breakpoint across multiple CSS files in the theme, including the base CSS framework. This matches with the standard Bootstrap md (medium) breakpoint of 768px.
The 48rem breakpoint used in the image sizes attribute equals 768px (48 * 16px base font size = 768px), showing consistency with the theme's responsive design system.
Breakpoint is correctly aligned with theme's responsive design
The sizes attribute using 48rem breakpoint is consistent with the theme's responsive design system that uses 768px (48rem) as a standard breakpoint across stylesheets.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other breakpoint definitions to ensure consistency
rg -g '*.{css,scss}' '48rem|768px'
Length of output: 157599
themes/beaver/layouts/partials/clients/grid-single.html (2)
29-31: LGTM: Using .RelPermalink is the preferred approach.
The change to use .RelPermalink instead of .Permalink for internal links is a good practice as it makes the site more portable across different environments.
Let's verify this change is consistent across other templates:
#!/bin/bash
# Description: Check for any remaining .Permalink usage in templates
# Expected: No occurrences of .Permalink for internal links
rg -g '*.html' '\.Permalink' themes/Line range hint 1-46: Verify if these changes belong in this PR.
This file contains URL structure changes that appear unrelated to the PR's stated objective of updating menu styles to match the design. Consider splitting these changes into a separate PR focused on URL structure improvements.
test/support/setup_capybara.rb (1)
22-22: LGTM! Disabling lazy image loading improves screenshot test reliability.
This change ensures that all images are loaded immediately during visual testing, preventing inconsistencies in screenshot comparisons that could occur with lazy-loaded images.
Let's verify if other test files are configured consistently:
✅ Verification successful
✓ Chrome configuration is properly centralized and consistent
The change to disable lazy image loading is correctly implemented in the single, centralized Chrome configuration file. The setup_capybara.rb file is the only place where Chrome options are configured, and it's used consistently for both desktop and mobile drivers through the register_chrome_driver function. No conflicting configurations exist elsewhere in the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other image loading configurations in test files
# that might conflict with this setting
# Search for any image loading related configurations
rg -i "lazyload|lazy.?load|loading.*lazy" --type ruby test/
Length of output: 61
Script:
#!/bin/bash
# Let's check for any other Capybara/Chrome configuration files and settings
# that might have similar browser configurations
# Search for Chrome-related configurations
rg -i "chrome.*options|chrome.*args|chrome.*preferences" test/
# Also check for any setup_capybara files
fd "setup.*capybara" test/
Length of output: 504
Script:
#!/bin/bash
# Let's examine the content of setup_capybara.rb to see if there are any
# other places where browser features are being configured
cat test/support/setup_capybara.rb
Length of output: 2708
themes/beaver/layouts/_default/list.html (1)
57-57: Verify the impact of switching to relative URLs.
The change from .Permalink to .RelPermalink is correct for maintaining consistent relative URLs. However, let's verify that this doesn't break any existing functionality.
themes/beaver/layouts/blog/list.html (1)
67-67: LGTM: URL handling improvement
The change from .Permalink to .RelPermalink is a good practice for generating relative URLs, improving portability across different environments.
themes/beaver/layouts/page/careers.html (1)
393-393: LGTM! Using .RelPermalink is the correct approach.
The change from .Permalink to .RelPermalink is a good practice in Hugo templates. .RelPermalink generates relative URLs which are more portable and work better with different base URL configurations.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
themes/beaver/layouts/_default/list.html (2)
Line range hint
2-13: Consider organizing CSS files by feature.Given that this PR focuses on menu styles, consider organizing the CSS files into logical groups (e.g., navigation, layout, components) to improve maintainability. This would make it easier to locate and modify menu-related styles in the future.
Consider this structure:
{{- $CSS := slice $dynamicCSS586 + // Navigation styles (resources.Get "css/navigation/menu.css") (resources.Get "css/navigation/base.css") + // Layout styles (resources.Get "css/layout/586.css") (resources.Get "css/layout/3114-layout.css") + // Components (resources.Get "css/components/single-post.css") (resources.Get "css/components/footer.css") | resources.Concat "css/blog.css" | postCSS | minify | fingerprint | resources.PostProcess}}
Line range hint
57-76: Enhance accessibility for blog post links.While the link structure is good, consider these accessibility improvements:
- Add aria-label to links opening in new tabs
- Ensure the image partial includes alt text
Apply these changes:
- <a class="link" target="_blank" rel="noopener noreferrer" href="{{ .RelPermalink }}"> + <a class="link" + target="_blank" + rel="noopener noreferrer" + href="{{ .RelPermalink }}" + aria-label="Read more about {{ .Title }} (opens in new tab)">Also, ensure the
img-cropped.htmlpartial includes proper alt text:- {{ partial "blog/img-cropped.html" (dict "Page" . "width" 180 "height" 180) }} + {{ partial "blog/img-cropped.html" (dict "Page" . "width" 180 "height" 180 "alt" .Title) }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
test/fixtures/screenshots/linux/desktop/blog/index/_pagination.pngis excluded by!**/*.pngtest/fixtures/screenshots/macos/desktop/blog/index/_pagination.pngis excluded by!**/*.png
📒 Files selected for processing (3)
themes/beaver/assets/css/navigation.css(6 hunks)themes/beaver/layouts/_default/list.html(1 hunks)themes/beaver/layouts/partials/blog/img-cropped.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- themes/beaver/assets/css/navigation.css
- themes/beaver/layouts/partials/blog/img-cropped.html
🔇 Additional comments (1)
themes/beaver/layouts/_default/list.html (1)
57-57: Verify the impact of changing to RelPermalink.
The switch from .Permalink to .RelPermalink is a good practice for internal links. However, we should verify that this change doesn't affect any existing bookmarks or SEO.
✅ Verification successful
Based on the shell script results, I can generate the final response:
Change to RelPermalink is safe and recommended
The switch from .Permalink to .RelPermalink for internal blog post links is a good practice. The search results show that absolute URLs (.Permalink) are only used in sharing buttons, meta tags, and JSON-LD where full URLs are required. The blog post links can safely use relative URLs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any hardcoded references to absolute URLs that might be affected
rg -g '*.html' -g '*.md' "https?://[^/]+/blog/"
# Check for potential mixed usage of Permalink and RelPermalink
rg -g '*.html' "\.Permalink"
Length of output: 58465
to prevent the crawler from parsing it and not downgrading our site. |
Recently, while we refactored, we changed the menu pretty hard. Maybe we need to revise our menu to follow the basic layout from our Adobe XD. We should not use fonts only.
We have such differences:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation