fix(components): resolve layout issue on GitHub star button (#7859) - #7899
fix(components): resolve layout issue on GitHub star button (#7859)#7899superezzdev wants to merge 5 commits into
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a styled GitHub star banner with cached repository star counts, integrates it into the Meshery hero section, revises feature-section and carousel styling, and corrects mobile banner spacing. ChangesMeshery landing page updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MesheryPage
participant GitHubStarButtonBanner
participant localStorage
participant GitHubAPI
MesheryPage->>GitHubStarButtonBanner: Render repository banner
GitHubStarButtonBanner->>localStorage: Read cached star count
alt Cache missing or expired
GitHubStarButtonBanner->>GitHubAPI: Fetch repository data
GitHubAPI-->>GitHubStarButtonBanner: Return stargazer count
GitHubStarButtonBanner->>localStorage: Store count and timestamp
end
GitHubStarButtonBanner-->>MesheryPage: Render formatted count or fallback
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes the GitHub Star Button layout issue on the Meshery page by correcting a CSS typo and replacing the previous “Star the Repository” button with a dedicated banner component that includes stable styling and star-count display.
Changes:
- Fixes a CSS margin typo affecting banner button spacing.
- Replaces the GitHub star CTA button with a new
GitHubStarButtonBannercomponent. - Adds client-side fetching + caching of GitHub star count and a custom styled banner button.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/sections/Meshery/meshery.style.js | Fixes a margin value typo that could break layout. |
| src/sections/Meshery/index.js | Swaps old GitHub star button for the new banner component and reformats JSX. |
| src/sections/Meshery/GitHubStarButton/GitHubStarButtonBanner.js | Introduces the new banner button with styling, GitHub API fetch, and localStorage caching. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sections/Meshery/GitHubStarButton/GitHubStarButtonBanner.js`:
- Around line 98-100: Update GitHubStarButtonBanner so its default url is
derived from the effective repo value, ensuring an overridden repo fetches,
labels, and links to the same repository; preserve the existing Meshery defaults
when neither prop is provided.
- Around line 75-79: Update the .banner-count styling to reserve a fixed width
sufficient for the selected formatted count, loading ellipsis, and “Star”
fallback. Keep the existing spacing and divider styling, ensuring the
inline-flex banner does not resize when the asynchronous count loads.
- Around line 107-156: Update the useEffect tied to repo so it resets the star
count when a new repository begins loading and tracks whether the effect is
still active. In the fetch success path and any related state updates, ignore
results after cleanup so responses from previous repos cannot overwrite the
current state; return cleanup that marks the request stale.
In `@src/sections/Meshery/meshery.style.js`:
- Line 148: Replace the hardcoded margin in the affected styled-components rule
with the appropriate existing theme spacing token, or define a new token if no
suitable one exists. Preserve the current spacing values and ensure the rule
accesses spacing through the theme.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a9e6bc57-23d6-4bd4-bbe5-0bf37bb41348
📒 Files selected for processing (3)
src/sections/Meshery/GitHubStarButton/GitHubStarButtonBanner.jssrc/sections/Meshery/index.jssrc/sections/Meshery/meshery.style.js
|
Hi @Maanvi212006, |
|
@superezzdev Keep in mind that in order to improve your code pull request, you must address all bot comments by accepting them with new changes or rejecting them. In both the cases, justify your choice by commenting below that review. Repeat this cycle until zero unresolved comments remain so your code is ready for human review. |
…sponsive controls Signed-off-by: GitHub <noreply@github.com>
…w, and focus state Signed-off-by: GitHub <noreply@github.com>
9e935d8 to
18a8297
Compare
Signed-off-by: GitHub <noreply@github.com>
18a8297 to
0626645
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/sections/Meshery/GitHubStarButton/GitHubStarButtonBanner.js (1)
6-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the responsive rules mobile-first.
The base styles are desktop values and the
max-widthblock overrides them for mobile. Set mobile values by default, then restore desktop values with@media (min-width: 769px).Proposed adjustment
- min-width: 200px; - padding: 12px 30px; + min-width: 180px; + padding: 12px 24px; ... - font-size: 15px; + font-size: 14px; ... .banner-count { - padding-left: 16px; - margin-left: 16px; + padding-left: 12px; + margin-left: 12px; border-left: 1px dashed ${({ theme }) => theme.bannerAccent || "`#2bb77c`"}; - min-width: 55px; + min-width: 48px; } - `@media` (max-width: 768px) { - min-width: 180px; - padding: 12px 24px; - font-size: 14px; + `@media` (min-width: 769px) { + min-width: 200px; + padding: 12px 30px; + font-size: 15px; .banner-count { - padding-left: 12px; - margin-left: 12px; - min-width: 48px; + padding-left: 16px; + margin-left: 16px; + min-width: 55px; } }As per coding guidelines, use “responsive mobile-first design.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sections/Meshery/GitHubStarButton/GitHubStarButtonBanner.js` around lines 6 - 106, Update the BannerButton responsive styles to use mobile-first defaults: move the current mobile min-width, padding, and font-size values into the base declarations, and replace the max-width media query with a min-width: 769px block that restores the desktop values. Apply the same inversion to the nested .banner-count spacing and min-width rules while preserving all other styling.Source: Coding guidelines
src/sections/Meshery/Features-section/features-section.style.js (2)
168-172: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDot thumbnails may overflow their 5rem-wide reserved area.
p imginside.slick-dots liis constrained only byheight: 3.5remwithwidth: auto. If the actual thumbnails (Slide1, etc., used ascustomPagingimages) are wider than tall, the rendered width at 3.5rem height could exceed the 5rem.slick-dotswidth, spilling over into the main carousel image area since there's nooverflow/max-widthclamp here.♻️ Suggested guard
p img { height: 3.5rem; width: auto; + max-width: 100%; + object-fit: cover; border-radius: 0.25rem; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sections/Meshery/Features-section/features-section.style.js` around lines 168 - 172, Update the p img rule within the slick-dots styling to constrain thumbnail width to the dots’ 5rem reserved area while preserving the existing 3.5rem height and auto sizing behavior where possible. Add the minimal overflow or max-width guard needed to prevent wide customPaging images such as Slide1 from spilling into the carousel area.
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded colors instead of theme tokens.
background-color: black,#c9fcf6(repeated),color: white(repeated across.section-data,.smp-section,.smp-section-data), andrgba(255,255,255,0.7)are hardcoded rather than sourced fromprops.theme, even thoughtheme.secondaryColoris correctly used elsewhere in this same file (e.g. lines 26, 33, 67, 163). This makes future theme/dark-mode changes error-prone and inconsistent.♻️ Example fix pattern
- background-color: black; + background-color: ${(props) => props.theme.primaryBackground || "black"}; ... - background: `#c9fcf6`; + background: ${(props) => props.theme.accentColor || "`#c9fcf6`"}; ... - h1, h2, p { color: white; } + h1, h2, p { color: ${(props) => props.theme.textOnDark || "white"}; }As per coding guidelines, "Use theme values for styled-components, maintain responsive design with media queries, and follow BEM-like naming for CSS classes when used."
Also applies to: 19-19, 40-40, 55-59, 95-98, 100-106, 187-187, 192-192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sections/Meshery/Features-section/features-section.style.js` at line 4, Replace the hardcoded color values throughout the styled components, including background-color, repeated `#c9fcf6` and white text colors, and rgba(255,255,255,0.7), with appropriate props.theme tokens. Update the affected selectors including .section-data, .smp-section, and .smp-section-data while preserving the existing theme.secondaryColor usage and responsive rules.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sections/Meshery/Features-section/features-section.style.js`:
- Around line 138-140: Update the .slick-arrow rule in the features section
styles to hide react-slick arrows reliably by preserving the display:none
declaration with !important, or disable arrow rendering through the arrows
option in the Features-section component.
In `@src/sections/Meshery/GitHubStarButton/GitHubStarButtonBanner.js`:
- Around line 141-143: Update the cache validity condition in the GitHub star
cache parsing flow to reject future-dated entries by requiring the timestamp age
to be non-negative as well as less than one hour. Preserve the existing numeric
count validation and expiration behavior.
---
Nitpick comments:
In `@src/sections/Meshery/Features-section/features-section.style.js`:
- Around line 168-172: Update the p img rule within the slick-dots styling to
constrain thumbnail width to the dots’ 5rem reserved area while preserving the
existing 3.5rem height and auto sizing behavior where possible. Add the minimal
overflow or max-width guard needed to prevent wide customPaging images such as
Slide1 from spilling into the carousel area.
- Line 4: Replace the hardcoded color values throughout the styled components,
including background-color, repeated `#c9fcf6` and white text colors, and
rgba(255,255,255,0.7), with appropriate props.theme tokens. Update the affected
selectors including .section-data, .smp-section, and .smp-section-data while
preserving the existing theme.secondaryColor usage and responsive rules.
In `@src/sections/Meshery/GitHubStarButton/GitHubStarButtonBanner.js`:
- Around line 6-106: Update the BannerButton responsive styles to use
mobile-first defaults: move the current mobile min-width, padding, and font-size
values into the base declarations, and replace the max-width media query with a
min-width: 769px block that restores the desktop values. Apply the same
inversion to the nested .banner-count spacing and min-width rules while
preserving all other styling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2bb00b86-a5b1-47bb-b7a3-9809995442f4
📒 Files selected for processing (4)
src/sections/Meshery/Features-section/features-section.style.jssrc/sections/Meshery/GitHubStarButton/GitHubStarButtonBanner.jssrc/sections/Meshery/index.jssrc/sections/Meshery/meshery.style.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/sections/Meshery/meshery.style.js
- src/sections/Meshery/index.js
…amp guard Signed-off-by: GitHub <noreply@github.com>
|
|
Thanks @superezzdev ! I will take a look |
|
Hi @Maanvi212006 |
Signed-off-by: 𝙰𝚁𝚈𝙰 𝚁𝙲𝙱 <248160976+superezzdev@users.noreply.github.com>
superezzdev
left a comment
superezzdev
left a comment
There was a problem hiding this comment.
Mistakenly commited in Github Button issue
this file changes was from Layout issue
Description
This PR fixes #7859 by resolving the layout rendering issue on the GitHub Star Button component.
Changes Made
Before
After
Notes for Reviewers
gatsby develop.Signed commits
Summary by CodeRabbit