fix: add responsiveness to mobile banner for imagine #2666
Conversation
WalkthroughThis change reworks the teaser banner component in a single Svelte file, transitioning from a two-column horizontal layout to a responsive stacked layout. The markup is reorganized into two stacked sections with alternating alignment based on viewport size. New CSS classes are introduced for the banner container with explicit width, height, and responsive padding/margin behavior. The logo styling is refactored from inline styles to a reusable container class. Typography and sizing are updated with responsive rules, while the underlying link behavior and semantic structure remain intact. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/routes/contact-us/enterprise/+page.svelte (1)
227-227: Consider alignment between placeholder and validation pattern.The placeholder
https://appwrite.iosuggests a full URL with protocol is expected, but the validation pattern on line 224 makes thehttps://prefix optional. Users might enter "appwrite.io" without the protocol and still pass validation.Ensure this behavior aligns with backend expectations and data processing requirements.
src/routes/(marketing)/(components)/teaser/teaser-banner.svelte (2)
122-146: Verify the font size dip at small breakpoint is intentional.The
.banner-text-introducingfont size decreases from 20px (mobile) to 18px (sm breakpoint), then increases back to 20px (md breakpoint). This creates an unusual "dip" in the middle range:
- < 640px: 20px
- 640px–767px: 18px
- ≥ 768px: 20px
Typically, font sizes either increase or remain constant as viewport width grows. Please verify this progression matches the design specification.
172-195: Consider simplifying logo-container sizing.Several observations:
Unusual responsive pattern: Like the text, logo dimensions decrease at the
smbreakpoint (143px → 120px) then increase again atmd(back to 143px), creating a size "dip."Sub-pixel precision: Values like
143.384px,35.785px, and1.348pxsuggest design tool exports, but browsers round to whole pixels anyway. This precision doesn't affect rendering.Redundant constraints: Setting explicit
width/heighton the container while usingw-auto h-autoon the<img>(line 77) may be redundant—the image's intrinsic dimensions or a single constraint should suffice.Consider:
- Verifying the sm → md size progression matches design intent
- Rounding dimensions to whole pixels for clarity
- Letting the image's natural aspect ratio dictate sizing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(marketing)/(components)/teaser/teaser-banner.svelte(4 hunks)src/routes/contact-us/enterprise/+page.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: assets
- GitHub Check: tests
🔇 Additional comments (2)
src/routes/(marketing)/(components)/teaser/teaser-banner.svelte (2)
54-54: LGTM: Responsive container classes.The combination of Tailwind utilities and custom
banner-containerclass provides appropriate responsive behavior with scaling padding and explicit height control at the medium breakpoint.
67-99: LGTM: Well-structured responsive layout.The refactored layout appropriately stacks content vertically on mobile (
flex-col) and switches to horizontal alignment on larger viewports (sm:flex-row). The semantic grouping of "Introducing + Logo" and "Build something real + Arrow" sections provides clear visual hierarchy across breakpoints.
| <input | ||
| required | ||
| pattern="^(https?:\/\/)?[\w\-]+(\.[\w\-]+)+(\/.*)?$" | ||
| pattern="^(https:\/\/www\.|https:\/\/)?([a-zA-Z0-9-]+(\.[a-zA-Z0-9-]+)+)[a-zA-Z0-9\-\._~:\/\?#[\]@!\$&'\(\)\*\+,;=.]*$" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and examine the actual file
echo "=== Finding the file ==="
find . -name "+page.svelte" -path "*/contact-us/enterprise/*" 2>/dev/null
echo -e "\n=== Reading the file around line 224 ==="
if [ -f "src/routes/contact-us/enterprise/+page.svelte" ]; then
sed -n '215,235p' "src/routes/contact-us/enterprise/+page.svelte" | cat -n
else
echo "File not found at expected location"
fiRepository: appwrite/website
Length of output: 1853
🏁 Script executed:
#!/bin/bash
# Test URL validation regex against common valid and invalid inputs
PATTERN='^(https:\/\/www\.|https:\/\/)?([a-zA-Z0-9-]+(\.[a-zA-Z0-9-]+)+)[a-zA-Z0-9\-\._~:\/\?#[\]@!\$&'"'"'\(\)\*\+,;=.]*$'
# Valid URLs that should match
echo "Testing valid URLs:"
echo "appwrite.io" | grep -P "$PATTERN" && echo "✓ appwrite.io matches" || echo "✗ appwrite.io doesn't match"
echo "https://appwrite.io" | grep -P "$PATTERN" && echo "✓ https://appwrite.io matches" || echo "✗ https://appwrite.io doesn't match"
echo "https://www.appwrite.io" | grep -P "$PATTERN" && echo "✓ https://www.appwrite.io matches" || echo "✗ https://www.appwrite.io doesn't match"
echo "https://appwrite.io/path" | grep -P "$PATTERN" && echo "✓ https://appwrite.io/path matches" || echo "✗ https://appwrite.io/path doesn't match"
echo "https://sub.appwrite.io" | grep -P "$PATTERN" && echo "✓ https://sub.appwrite.io matches" || echo "✗ https://sub.appwrite.io doesn't match"
# Invalid URLs that should NOT match
echo -e "\nTesting invalid URLs:"
echo "not-a-url" | grep -P "$PATTERN" && echo "✗ not-a-url matches (should reject)" || echo "✓ not-a-url rejected"
echo "http://appwrite" | grep -P "$PATTERN" && echo "✗ http://appwrite matches (should reject)" || echo "✓ http://appwrite rejected"
echo "ftp://appwrite.io" | grep -P "$PATTERN" && echo "✗ ftp://appwrite.io matches (should reject)" || echo "✓ ftp://appwrite.io rejected"
# Edge cases
echo -e "\nTesting edge cases:"
echo "https://appwrite.io:8080" | grep -P "$PATTERN" && echo "✓ Port number matches" || echo "✗ Port number doesn't match"
echo "appwrite.io" | grep -P "$PATTERN" && echo "! No protocol matches (is this intended?)" || echo "! No protocol rejected"Repository: appwrite/website
Length of output: 561
Address optional protocol handling in URL validation regex.
The regex pattern allows URLs without a protocol (e.g., "appwrite.io" matches), which conflicts with your placeholder showing "https://appwrite.io" as the expected format. If only HTTPS URLs are intended, make the protocol required by removing the trailing ?:
pattern="^https:\/\/(www\.)?([a-zA-Z0-9-]+(\.[a-zA-Z0-9-]+)+)..."
The regex correctly handles port numbers, rejects non-HTTPS protocols, and validates domain structure. However, the character class escaping is inconsistent—within [...], most special characters don't require backslashes. Consider simplifying or switching to the browser's native type="url" validation for better maintainability.
🤖 Prompt for AI Agents
In src/routes/contact-us/enterprise/+page.svelte around line 224, the URL
validation regex currently permits URLs without a protocol which conflicts with
the placeholder expecting "https://..."; make the protocol mandatory by changing
the pattern to require the "https://" prefix (remove the optional `?`), adjust
the start of the pattern to ^https:\/\/(www\.)?..., keep validation for domain
and optional port/path but ensure only https is accepted, and while editing
simplify the character-class escaping inside [...] (or switch the input to use
the browser's native type="url" for maintainability).
HarshMN2345
force-pushed
the
fix-mobile-responsiveness-for-imagine-banner-in-website
branch
from
4ec211a to
ea7dc24
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/routes/(marketing)/(components)/teaser/teaser-banner.svelte (2)
87-87: Verify arrow animation direction.The arrow icon uses
group-hover:-translate-x-0.5which translates the arrow left on hover. For a right-pointing arrow, users typically expect it to move right (positive x direction).If the intention is to create a "pull" effect, this is acceptable, but please confirm this matches the design intent.
If you want the arrow to move right on hover, apply this diff:
- class="h-4 w-4 flex-shrink-0 transition group-hover:-translate-x-0.5 sm:h-5 sm:w-5" + class="h-4 w-4 flex-shrink-0 transition group-hover:translate-x-0.5 sm:h-5 sm:w-5"</comment_end>
105-115: Fixed width causes overflow on narrow devices (duplicate concern).As flagged in the previous review, the fixed
width: 360pxat line 106 will cause horizontal overflow on devices narrower than 360px, such as iPhone SE (320px) and Galaxy Fold (280px closed).The previous review suggestion to use
max-width: 360pxwithwidth: 100%remains valid and should be implemented.Apply this diff as suggested in the previous review:
.banner-container { - width: 360px; + width: 100%; + max-width: 360px; height: 110px; }</comment_end>
🧹 Nitpick comments (2)
src/routes/(marketing)/(components)/teaser/teaser-banner.svelte (2)
67-69: Consider removing redundant gap class.The
sm:gap-4andmd:gap-[1rem]both evaluate to 16px (sincegap-4= 1rem), making themd:gap-[1rem]class redundant.Apply this diff to simplify:
- class="relative z-10 mx-auto flex w-full flex-col items-center justify-center gap-1 [max-inline-size:108rem] sm:flex-row sm:items-center sm:justify-between sm:gap-4 md:gap-[1rem]" + class="relative z-10 mx-auto flex w-full flex-col items-center justify-center gap-1 [max-inline-size:108rem] sm:flex-row sm:items-center sm:justify-between sm:gap-4"</comment_end>
172-195: Consider rounding sub-pixel dimensions to whole pixels.The logo container uses highly precise sub-pixel values (e.g.,
143.384px,35.785px,1.348px,0.634px,10.75px), likely exported from a design tool. Browsers typically round these values during rendering, so the extra precision may not provide visual benefit.For cleaner, more maintainable code, consider rounding to whole pixels unless the design explicitly requires sub-pixel accuracy.
Example:
.logo-container { display: flex; - width: 143.384px; - height: 35.785px; - padding: 1.348px 0.634px 1.348px 0; + width: 143px; + height: 36px; + padding: 1px 1px 1px 0; justify-content: center; align-items: flex-end; - gap: 10.75px; + gap: 11px; flex-shrink: 0; }</comment_end>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(marketing)/(components)/teaser/teaser-banner.svelte(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests
- GitHub Check: assets
- GitHub Check: build
🔇 Additional comments (2)
src/routes/(marketing)/(components)/teaser/teaser-banner.svelte (2)
71-79: LGTM!The top line section structure with introducing text and logo is well-organized and uses appropriate responsive classes.
</comment_end>
122-146: Verify the unusual responsive font sizing pattern.Both
.banner-text-introducingand.banner-text-rightuse an uncommon responsive pattern where font sizes decrease at thesmbreakpoint and then increase again atmd:
- Introducing: 20px (mobile) → 18px (sm) → 20px (md)
- Right text: 16px (mobile) → 15px (sm) → 16px (md)
Typically, responsive typography scales progressively upward as viewport widens. This down-then-up pattern is unusual. If this matches your design specifications, no action is needed. Otherwise, consider a progressive scale.
</comment_end>
Also applies to: 148-170
HarshMN2345
deleted the
fix-mobile-responsiveness-for-imagine-banner-in-website
branch
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.