fix(ui): responsivness of kmesh website#275
Conversation
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
✅ Deploy Preview for kmesh-net ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary of ChangesHello @yashisrani, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the responsiveness of the kmesh website's user interface. It introduces targeted CSS adjustments across several key components, including the 'About' section, content sliders, and the main navigation bar. The changes ensure that critical elements adapt gracefully to different screen sizes, providing a more consistent and user-friendly experience on both desktop and mobile devices by adjusting font sizes, element visibility, and layout. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several CSS changes to improve the responsiveness of the website, particularly for the navigation bar and content sections on different screen sizes. The changes are generally moving in the right direction. However, I've identified a few areas in the SCSS and CSS files where the implementation could be improved for better maintainability and to fix a bug related to media query ordering that breaks styling on smaller viewports. My comments focus on simplifying redundant media queries, fixing the style override bug, and avoiding the use of !important where possible.
| @media (min-width: 1300px) and (max-width: 1515px) { | ||
| .row { | ||
| .description { | ||
| font-size: 1.1rem; | ||
| } | ||
| .name { | ||
| font-size: 1.8em; | ||
| } | ||
| .jobTitle { | ||
| font-size: 0.9rem; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 1300px) { | ||
| .row { | ||
| .description { | ||
| font-size: 1.1rem; | ||
| } | ||
| .name { | ||
| font-size: 1.8em; | ||
| } | ||
| .jobTitle { | ||
| font-size: 0.9rem; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The media queries added here have issues with redundancy and ordering, which causes incorrect styling. The @media (max-width: 1300px) block is redundant as it contains the same styles as the (min-width: 1300px) and (max-width: 1515px) block. More critically, because it appears last in the source order, it overrides the more specific styles for max-width: 1024px and max-width: 435px, breaking the responsive design for smaller screens. Please refactor these queries to have clear, non-overlapping ranges or ensure they are ordered correctly (e.g., from largest max-width to smallest).
| @media (max-width: 1230px) and (min-width: 993px){ | ||
| .slick-slider .slick-item { | ||
| .main-title { | ||
| font-size: 2rem; | ||
| } | ||
| .sub-title { | ||
| font-size: 1.3rem; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 992px) { | ||
| .slick-slider .slick-item { | ||
| .main-title { | ||
| font-size: 2rem; | ||
| } | ||
| .sub-title { | ||
| font-size: 1.3rem; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
These two media queries for (max-width: 1230px) and (min-width: 993px) and (max-width: 992px) have identical styles. Since their ranges are contiguous, they can be combined into a single, simpler media query to improve readability and maintainability.
| @media (max-width: 1230px) and (min-width: 993px){ | |
| .slick-slider .slick-item { | |
| .main-title { | |
| font-size: 2rem; | |
| } | |
| .sub-title { | |
| font-size: 1.3rem; | |
| } | |
| } | |
| } | |
| @media (max-width: 992px) { | |
| .slick-slider .slick-item { | |
| .main-title { | |
| font-size: 2rem; | |
| } | |
| .sub-title { | |
| font-size: 1.3rem; | |
| } | |
| } | |
| } | |
| @media (max-width: 1230px) { | |
| .slick-slider .slick-item { | |
| .main-title { | |
| font-size: 2rem; | |
| } | |
| .sub-title { | |
| font-size: 1.3rem; | |
| } | |
| } | |
| } |
| @media (max-width: 768px) and (min-width: 565px) { | ||
| .slick-slider .slick-item { | ||
| .main-title { | ||
| font-size: 1.4rem; | ||
| } | ||
| .sub-title { | ||
| font-size: 1rem; | ||
| } | ||
| margin-left: auto; | ||
| margin-right: auto; | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 564px) { | ||
| .slick-slider .slick-item { | ||
| .main-title { | ||
| font-size: 1.4rem; | ||
| } | ||
| .sub-title { | ||
| font-size: 1rem; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The font-size rules are duplicated across these two media queries. You can combine them into a single (max-width: 768px) query to avoid repetition, and keep the margin rules in their specific range for better maintainability.
@media (max-width: 768px) {
.slick-slider .slick-item {
.main-title {
font-size: 1.4rem;
}
.sub-title {
font-size: 1rem;
}
}
}
@media (max-width: 768px) and (min-width: 565px) {
.slick-slider .slick-item {
margin-left: auto;
margin-right: auto;
}
}| @media (min-width: 997px) and (max-width: 1245px) { | ||
| /* hide all navbar links except the logo image */ | ||
| .navbar__item:not(.navbar__logo):not(img), | ||
| .navbar__link:not(.navbar__logo):not(img) { | ||
| display: none !important; | ||
| } | ||
|
|
||
| /* make sure logo, toggle, search stay visible */ | ||
| .navbar__logo, | ||
| .navbar__logo img, | ||
| .navbar__toggle, | ||
| .navbar__search, | ||
| .DocSearch-Button { | ||
| display: inline-block !important; | ||
| } | ||
|
|
||
| /* push logo to the far left */ | ||
| .navbar__brand { | ||
| margin-right: auto; | ||
| } | ||
|
|
||
| /* tidy sizes */ | ||
| .navbar__logo { height: 1.8rem; } | ||
| .navbar__title { font-size: 1.1rem; } | ||
| } | ||
|
|
||
| /* mobile sidebar : social icons in a ROW */ | ||
| @media (max-width: 996px) { | ||
| .navbar-sidebar__items .navbar__items--right, | ||
| .navbar-sidebar__items .navbar__items--right .navbar__item { | ||
| display: flex !important; | ||
| flex-direction: row !important; | ||
| gap: 0.75rem; /* space between icons */ | ||
| align-items: center; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The use of !important is generally discouraged as it can lead to maintenance issues and make debugging styles harder. It's present in both new media queries (lines 139, 148, 165, 166). While sometimes necessary to override library styles like those in Docusaurus, it's worth investigating if you can achieve the same result by increasing selector specificity. This would make the CSS more robust and easier to reason about.
There was a problem hiding this comment.
Pull request overview
This PR improves the site’s responsive layout (notably navbar behavior, the homepage slider typography, and the About section text sizing) to better fit mid-sized and small screens.
Changes:
- Add navbar/media-query overrides for intermediate widths and mobile sidebar icon layout.
- Adjust slider alignment styles and introduce additional responsive typography breakpoints.
- Add multiple About-section media queries to tune text sizes across screen widths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/css/custom.css |
Adds responsive navbar rules and mobile sidebar icon-row layout. |
src/components/slider/index.scss |
Fixes alignment typo, refactors button selector, and adds responsive typography/padding rules. |
src/components/About/styles.scss |
Adds several breakpoints to adjust About section text sizing on smaller/larger screens. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @media (min-width: 1300px) and (max-width: 1515px) { | ||
| .row { | ||
| .description { | ||
| font-size: 1.1rem; | ||
| } | ||
| .name { | ||
| font-size: 1.8em; | ||
| } | ||
| .jobTitle { | ||
| font-size: 0.9rem; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 1300px) { | ||
| .row { | ||
| .description { | ||
| font-size: 1.1rem; | ||
| } | ||
| .name { | ||
| font-size: 1.8em; | ||
| } | ||
| .jobTitle { | ||
| font-size: 0.9rem; | ||
| } | ||
| } |
There was a problem hiding this comment.
These two media queries apply the same font-size values and overlap at exactly 1300px. Consider consolidating into non-overlapping ranges (or a single rule with clearer breakpoints) to avoid redundant overrides and make future adjustments less error-prone.
| @media (max-width: 1230px) and (min-width: 993px){ | ||
| .slick-slider .slick-item { | ||
| .main-title { | ||
| font-size: 2rem; | ||
| } | ||
| .sub-title { | ||
| font-size: 1.3rem; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 992px) { | ||
| .slick-slider .slick-item { | ||
| .main-title { | ||
| font-size: 2rem; | ||
| } | ||
| .sub-title { | ||
| font-size: 1.3rem; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The (max-width: 1230px) and (min-width: 993px) and (max-width: 992px) media queries set identical .main-title/.sub-title sizes. This duplication makes it easy for the breakpoints to drift; consider merging into a single @media (max-width: 1230px) rule (and then override again only where sizes actually change).
| padding-right: 20px; | ||
| padding-left: 20px; | ||
| margin: 0 0.25rem; | ||
| @media (max-width: 1230px) and (min-width: 993px){ |
There was a problem hiding this comment.
Formatting nit: this media query is missing a space before { (compare other media queries in this repo, e.g. src/components/sectionContainer/styles.scss:26 and src/components/About/styles.scss:5, which use ) {). Aligning the formatting helps keep future diffs smaller.
| @media (max-width: 1230px) and (min-width: 993px){ | |
| @media (max-width: 1230px) and (min-width: 993px) { |
| @media (max-width: 435px) { | ||
| .row { | ||
| .description { | ||
| flex: 0 0 66.666667%; | ||
| max-width: 66.666667%; | ||
| font-size: 1rem; | ||
| } | ||
|
|
||
| .name { | ||
| font-size: 1.5em; | ||
| font-weight: 400; | ||
| margin: 20px 0 10px 0; | ||
| } | ||
|
|
||
| .jobTitle { | ||
| font-size: 0.5rem; | ||
| font-weight: 300; | ||
| margin: 0px 0 10px 0; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 1024px) { | ||
| .row { | ||
| .description { | ||
| font-size: 1rem; | ||
| } | ||
| .name { | ||
| font-size: 1.5em; | ||
| } | ||
| .jobTitle { | ||
| font-size: 0.7rem; | ||
| } | ||
| } |
There was a problem hiding this comment.
The @media (max-width: 1024px) block comes after the more specific @media (max-width: 435px) block, so at <=435px the later rules override the intended small-screen tweaks (e.g., .jobTitle becomes 0.7rem instead of 0.5rem). Reorder the media queries (smallest last) or add a min-width range so the <=435px overrides actually win.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LiZhenCheng9527 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Before:
After:
Before:
After:
