fix: updated the Github Button to IconGithub Button when screen size becomes small #4580
Conversation
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
BensOmnitrix
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. |
Contributor
WalkthroughGithubButton now renders a full Button on large viewports and an icon-only Link on smaller viewports using Tailwind visibility utilities and Next.js Link; Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant GithubButton
participant NextLink
participant Icon
rect rgba(135,206,235,0.12)
Browser->>GithubButton: render (with viewport size)
alt viewport >= xl
GithubButton->>GithubButton: render full Button (visible at xl and up)
note right of GithubButton `#eef6ff`: Desktop flow
else viewport < xl
GithubButton->>NextLink: render Link with Icon (icon-only)
NextLink->>Icon: show GitHub icon
note right of NextLink `#fff8e6`: Mobile flow (icon-only)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2024-10-11T10:46:58.882ZApplied to files:
📚 Learning: 2024-10-11T07:38:35.745ZApplied to files:
📚 Learning: 2024-10-11T11:32:30.226ZApplied to files:
🧬 Code graph analysis (1)components/buttons/GithubButton.tsx (2)
🔇 Additional comments (1)
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 |
BensOmnitrix
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4580 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 799 799
Branches 146 146
=========================================
Hits 799 799 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BensOmnitrix
changed the title
Contributor
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4580--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/buttons/GithubButton.tsx (1)
37-37: Consider consistent spacing between responsive variants.The desktop Button uses
ml-1while the mobile icon Link usesml-2, creating inconsistent left margins between the two responsive states. Additionally, the icon sizes differ (size-6in the Button vssize-8in the mobile Link).While the larger mobile icon might be intentional for better touch-target visibility, the margin inconsistency could create a visual shift when resizing the viewport.
Consider standardizing the left margin to ensure consistent spacing:
- className={`${className} ml-1 hidden xl:inline-block`} + className={`${className} ml-2 hidden xl:inline-block`}Or if
ml-1is preferred:- <IconGithub className='-mt-1 ml-2 inline-block size-8 xl:hidden' /> + <IconGithub className='-mt-1 ml-1 inline-block size-8 xl:hidden' />Also applies to: 41-41, 47-47
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/buttons/GithubButton.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
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.
📚 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/buttons/GithubButton.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/buttons/GithubButton.tsx
📚 Learning: 2024-10-11T11:32:30.226Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Applied to files:
components/buttons/GithubButton.tsx
🧬 Code graph analysis (1)
components/buttons/GithubButton.tsx (2)
components/buttons/Button.tsx (1)
Button(53-106)components/icons/Github.tsx (1)
IconGithub(7-13)
⏰ 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). (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Lighthouse CI
🔇 Additional comments (1)
components/buttons/GithubButton.tsx (1)
1-1: LGTM!The Link import is correctly added to support the responsive icon-only navigation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/index.ts (1)
62-62: Reasonable defensive check, but unrelated to PR scope.The added null/undefined guard is a reasonable defensive improvement. However, this change appears unrelated to the PR's stated objective of making the GitHub button responsive on small screens.
Consider separating unrelated changes into different PRs to maintain clear change tracking and make reviews more focused.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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). (5)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - windows-latest
Author
|
/ready-to-merge |
Member
|
@BensOmnitrix tests are failing. Please look into it. |
Author
|
@princerajpoot20 All the checks have been passed. Kindly check and approve if it looks good 😊 |
Member
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
Fixes [#4575 ]
-3. Also encapsulated the IconButton in the Link so that after clicking it one could be redirected to the asyncapi github repo.
Final Look
Related issue(s)
Summary by CodeRabbit
Style
Bug Fixes