fix(ui): responsivness of Kmesh website#274
fix(ui): responsivness of Kmesh website#274yashisrani wants to merge 2 commits intokmesh-net:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ 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 addresses UI responsiveness issues across the Kmesh website, primarily by introducing and refining media queries in various SCSS and CSS files. These changes ensure that key components like the About section, image sliders, and the navigation bar adapt gracefully and display optimally on different screen sizes, improving the overall user experience. 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
The pull request introduces several responsiveness improvements across the Kmesh website by adding new media queries and refining existing styles in SCSS files. The changes address various screen sizes, ensuring a better user experience on different devices. Additionally, some minor stylistic adjustments and typo fixes have been made, contributing to overall code quality. However, there are opportunities to optimize the media query structure in src/components/About/styles.scss and src/components/slider/index.scss to prevent redundancy and ensure a more predictable CSS cascade.
| @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 query @media (max-width: 1300px) (lines 121-133) has identical styles to the @media (min-width: 1300px) and (max-width: 1515px) rule (lines 107-118). This creates redundancy and can make the CSS harder to maintain. If the intent is for these styles to apply to the combined range of (min-width: 1025px) and (max-width: 1515px) (assuming the max-width: 1024px rule is the next breakpoint), consider combining these into a single, more comprehensive media query to avoid duplication.
| @media (max-width: 1300px) { | |
| .row { | |
| .description { | |
| font-size: 1.1rem; | |
| } | |
| .name { | |
| font-size: 1.8em; | |
| } | |
| .jobTitle { | |
| font-size: 0.9rem; | |
| } | |
| } | |
| } | |
| @media (min-width: 1025px) and (max-width: 1515px) { | |
| .row { | |
| .description { | |
| font-size: 1.1rem; | |
| } | |
| .name { | |
| font-size: 1.8em; | |
| } | |
| .jobTitle { | |
| font-size: 0.9rem; | |
| } | |
| } | |
| } |
| @media (max-width: 768px) { | ||
| .slick-slider .slick-item { | ||
| padding-right: 20px; | ||
| padding-left: 20px; | ||
| margin: 0 0.25rem; | ||
| .title { | ||
| text-align: center; | ||
| } |
There was a problem hiding this comment.
The @media (max-width: 768px) rule is placed after the more specific @media (max-width: 768px) and (min-width: 565px) rule. While the properties might not directly conflict, it's generally best practice to order media queries from broadest to most specific (for max-width queries) or use non-overlapping ranges. This ensures a clearer and more predictable CSS cascade, making the stylesheet easier to understand and maintain.
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to improve the responsiveness of the Kmesh website by adding and refining media queries across CSS and SCSS files. The changes address navbar display issues at medium screen sizes, fix existing bugs in the slider component, and add responsive font sizing for various breakpoints.
Changes:
- Added navbar responsiveness for medium screens (997-1245px) to show only logo, toggle, and search
- Fixed CSS bugs in slider component (typo "centers" → "center", invalid "position: position" declarations)
- Added multiple responsive breakpoints for slider font sizes and About section styling
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/css/custom.css | Added media queries for navbar at 997-1245px and mobile sidebar social icons layout |
| src/components/slider/index.scss | Fixed CSS bugs, refactored button styles, added responsive font sizing across 5 breakpoints |
| src/components/About/styles.scss | Added responsive font sizing across 4 new breakpoints for name, jobTitle, and description |
Comments suppressed due to low confidence (1)
src/components/slider/index.scss:144
- The media queries have overlapping ranges that will cause conflicts. The
@media (max-width: 768px)block (lines 135-144) overlaps with both@media (max-width: 768px) and (min-width: 565px)(lines 111-122) and@media (max-width: 564px)(lines 124-133). At exactly 768px down to 565px, both the specific range query and the general max-width query will apply simultaneously. Consider using more exclusive ranges (e.g., max-width: 564px for the general query) or ensuring the general query comes first in the cascade.
@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;
}
}
}
@media (max-width: 768px) {
.slick-slider .slick-item {
padding-right: 20px;
padding-left: 20px;
margin: 0 0.25rem;
.title {
text-align: center;
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .slick-slider .slick-item { | ||
| .main-title { | ||
| font-size: 1.4rem; | ||
| } |
There was a problem hiding this comment.
Trailing whitespace after the closing brace. This is a minor formatting issue but should be cleaned up for consistency.
| } | |
| } |
| } | ||
|
|
||
| .jobTitle { | ||
| font-size: 0.5rem; |
There was a problem hiding this comment.
The font size for jobTitle at this breakpoint is extremely small (0.5rem). This could make the text nearly unreadable on small screens. Consider using a minimum of 0.7rem or 0.75rem for better accessibility and readability.
| font-size: 0.5rem; | |
| font-size: 0.7rem; |
| @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 media queries define identical styling rules. The @media (max-width: 992px) at lines 100-109 and @media (max-width: 1230px) and (min-width: 993px) at lines 89-98 set the exact same font sizes (main-title: 2rem, sub-title: 1.3rem). Consider consolidating these into a single media query @media (max-width: 1230px) to avoid duplication and improve maintainability.
| @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; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @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 query cascade is problematic. At viewport widths ≤435px, three media queries will match and apply in order: lines 71-91, 93-105, and 121-133. Due to CSS cascade order, the rules from the last query (121-133) will override the earlier ones, making the specific styling for small screens at lines 71-91 ineffective. The intended font sizes for very small screens (jobTitle: 0.5rem, description: 1rem, name: 1.5em) will be overridden by the larger breakpoint styles (jobTitle: 0.9rem, description: 1.1rem, name: 1.8em). Consider using min-width constraints or reordering so that larger breakpoints come first and smaller breakpoints come last.
| margin-left: auto; | ||
| margin-right: auto; |
There was a problem hiding this comment.
The margin-left: auto and margin-right: auto properties (lines 119-120) are applied directly to .slick-item but should likely be applied within the nested .title selector or another appropriate child element. Currently, these margin properties are at the same nesting level as the .main-title and .sub-title selectors, which could cause unintended layout effects on the slider item itself rather than its content.
| margin-left: auto; | |
| margin-right: auto; | |
| .title { | |
| margin-left: auto; | |
| margin-right: auto; | |
| } |
No description provided.