fix(ui): align newsroom arrows, fix navbar overlap on multiple pages, make scroll-to-top theme-aware (fix #4522) #4524
Conversation
…scroll-to-top theme aware (fix asyncapi#4522)
abhishekkumawat-47
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
|
Warning Rate limit exceeded@abhishekkumawat-47 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis PR updates several UI spacing and styling details: replaces the scroll-to-top image with a new SVG icon component, tweaks button shadows/transitions, adjusts AnnouncementHero usage and banner spacing, and applies multiple top-margin/layout tweaks across pages and newsroom carousel controls. Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Page Component
participant ScrollButton as ScrollButton
participant Icon as IconArrowUp (SVG)
participant Theme as CSS (currentColor)
Page->>ScrollButton: Render ScrollButton
ScrollButton->>Icon: Render IconArrowUp with className
Icon->>Theme: SVG uses currentColor to inherit text color
Theme-->>Icon: Provides theme color
Icon-->>ScrollButton: Themed SVG rendered
ScrollButton-->>Page: Displays scroll-to-top button
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Areas to check:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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 |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4524--asyncapi-website.netlify.app/ |
abhishekkumawat-47
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/campaigns/AnnouncementHero.tsx (1)
24-24: Remove unnecessary dependency from useMemo.The
bannersimport is a constant reference and doesn't need to be in the dependency array.Apply this diff:
- const visibleBanners = useMemo(() => banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)), [banners]); + const visibleBanners = useMemo(() => banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)), []);components/buttons/ScrollButton.tsx (1)
10-10: Remove unused variable.The
scrollImagevariable is no longer used after replacing the image-based icon with the SVG component.Apply this diff:
- const scrollImage = '/img/loaders/scroll.svg';Based on the pipeline failure logs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/Hero.tsx(1 hunks)components/buttons/ScrollButton.tsx(2 hunks)components/campaigns/AnnouncementHero.tsx(1 hunks)components/icons/Scroll-up-arrow.tsx(1 hunks)components/newsroom/NewsroomBlogPosts.tsx(1 hunks)components/newsroom/NewsroomYoutube.tsx(1 hunks)pages/404.tsx(1 hunks)pages/[lang]/index.tsx(1 hunks)pages/community/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/newsroom/NewsroomBlogPosts.tsx
📚 Learning: 2024-10-11T07:38:35.745Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:38:35.745Z
Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
Applied to files:
components/newsroom/NewsroomBlogPosts.tsx
🧬 Code graph analysis (4)
components/Hero.tsx (1)
components/campaigns/AnnouncementHero.tsx (1)
AnnouncementHero(21-112)
components/newsroom/NewsroomBlogPosts.tsx (1)
components/icons/ArrowLeft.tsx (1)
ArrowLeft(7-17)
components/buttons/ScrollButton.tsx (1)
components/icons/ArrowUp.tsx (1)
IconArrowUp(7-14)
components/newsroom/NewsroomYoutube.tsx (1)
components/icons/ArrowLeft.tsx (1)
ArrowLeft(7-17)
🪛 GitHub Actions: PR testing - if Node project
components/campaigns/AnnouncementHero.tsx
[warning] 24-24: React Hook useMemo has an unnecessary dependency: 'banners'.
components/icons/Scroll-up-arrow.tsx
[error] 10-10: Replace "currentColor" with 'currentColor'
[error] 10-10: Unexpected usage of doublequote. jsx-quotes
[error] 11-11: Replace "0 0 20 20" with '0 0 20 20'
[error] 11-11: Unexpected usage of doublequote. jsx-quotes
[error] 12-12: Replace "http://www.w3.org/2000/svg" with 'http://www.w3.org/2000/svg'
[error] 12-12: Unexpected usage of doublequote. jsx-quotes
[error] 15-15: Replace "evenodd" with 'evenodd'
[error] 15-15: Unexpected usage of doublequote. jsx-quotes
[error] 16-16: This line has a length of 140. Maximum allowed is 120.
[error] 16-16: Replace the long string with a properly quoted string using single quotes.
[error] 16-16: Unexpected usage of doublequote. jsx-quotes
[error] 17-17: Replace "evenodd" with 'evenodd'
[error] 17-17: Unexpected usage of doublequote. jsx-quotes
components/buttons/ScrollButton.tsx
[error] 1-1: autofix required: sort these imports. Run autofix for simple-import-sort/imports.
[error] 10-10: 'scrollImage' is assigned a value but never used. (no-unused-vars)
[error] 10-10: 'scrollImage' is assigned a value but never used. no-unused-vars
[error] 10-10: Unused variable detected: 'scrollImage'.
[error] 36-36: Replace with
[error] 36-36: Unexpected usage of doublequote. jsx-quotes
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lighthouse CI
🔇 Additional comments (8)
components/Hero.tsx (1)
32-32: LGTM: Cleaner component usage.Removing the
classNameprop simplifies the component usage, as the spacing is now handled internally byAnnouncementHero(which addsmt-5to its banner container).components/campaigns/AnnouncementHero.tsx (1)
64-64: LGTM: Spacing fix applied correctly.The
mt-5addition provides the top margin needed to prevent navbar overlap, aligning with the PR objectives.pages/[lang]/index.tsx (1)
45-45: LGTM: Negative margin correctly addresses navbar overlap.The
-mt-6adjustment pulls the content up to prevent navbar from overlapping the hero section, which aligns with the PR objectives.pages/community/index.tsx (1)
36-36: LGTM: Removed excessive top margin.Removing
mt-15eliminates unnecessary spacing on small screens, improving the layout consistency as part of the overall spacing fixes.components/newsroom/NewsroomBlogPosts.tsx (1)
60-63: LGTM: Improved slider arrow styling.The padding adjustment and icon size increase improve the visual consistency and alignment of the slider arrows, addressing the newsroom slider arrow misalignment issue mentioned in the PR objectives.
components/newsroom/NewsroomYoutube.tsx (1)
54-57: LGTM: Consistent slider arrow styling.The styling updates match those in
NewsroomBlogPosts.tsx, ensuring uniform slider arrow appearance across all newsroom components.pages/404.tsx (1)
15-15: LGTM: Spacing adjustment for 404 page.The negative margin adjustment improves the vertical spacing on the error page, consistent with the overall spacing fixes in this PR.
components/buttons/ScrollButton.tsx (1)
33-33: LGTM: Button styling refinements enhance the UI.The updated shadow, transition duration, and easing provide a more polished, responsive feel for the scroll-to-top button.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/buttons/ScrollButton.tsx (1)
11-11: Remove unused variable.The
scrollImagevariable is no longer used since the image-based icon was replaced with theIconArrowUpcomponent.Apply this diff:
- const scrollImage = '/img/loaders/scroll.svg';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/buttons/ScrollButton.tsx(2 hunks)components/icons/Scroll-up-arrow.tsx(1 hunks)next-env.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- next-env.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- components/icons/Scroll-up-arrow.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
components/buttons/ScrollButton.tsx (1)
components/icons/ArrowUp.tsx (1)
IconArrowUp(7-14)
🪛 Biome (2.1.2)
components/buttons/ScrollButton.tsx
[error] 34-34: This JSX property is assigned multiple times.
This attribute is assigned again here.
(lint/suspicious/noDuplicateJsxProps)
🪛 GitHub Actions: PR testing - if Node project
components/buttons/ScrollButton.tsx
[error] 11-11: No duplicate props allowed
[error] 11-11: scrollImage is assigned a value but never used. (no-unused-vars)
🔇 Additional comments (1)
components/buttons/ScrollButton.tsx (1)
38-38: Theme-aware icon implementation looks good.The
IconArrowUpcomponent is correctly implemented with theme-aware styling. The use oftext-gray-900andhover:text-whiteensures the icon adapts appropriately when the background changes on hover, addressing the PR objective for theme awareness.
abhishekkumawat-47
force-pushed
the
fix/4522-slider-arrow
branch
from
ab2837a to
db0dd6f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4524 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 780 780
Branches 144 144
=========================================
Hits 780 780 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
abhishekkumawat-47
force-pushed
the
fix/4522-slider-arrow
branch
6 times, most recently
from
3fe99ca to
aad01ed
Compare
2 participants
Description
Visual Comparisons
🎥 Video Comparison
Before-Scroll-up-arrow.mp4
Scroll-up-arrow.mp4
Related issue(s)
Fixes #4522
Summary by CodeRabbit