fix(ui): clean navbar for tablet view & responsive slider text #251
Conversation
Signed-off-by: Yash Israni <[email protected]>
✅ Deploy Preview for kmesh-net ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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 enhances the website's responsiveness, particularly for tablet and mobile users. It addresses layout issues in the navigation bar by ensuring a clean, single-line display on specific tablet screen widths and implements smooth font size adjustments for slider text to maintain readability and prevent content overflow on smaller devices. These changes aim to provide a more polished and adaptive user experience. Highlights
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 does a great job of improving the user interface's responsiveness on tablet and mobile devices, particularly with the navbar and slider text. The cleanup of invalid CSS properties like align-items: centers and position: position is also a good improvement. I've identified a few areas in the new CSS rules where we can improve maintainability and fix some logical errors. My main concerns are around the ordering and structure of media queries, which in one case leads to styles not being applied as intended, and the use of !important which can make future styling changes more difficult. Please see my detailed comments below.
| @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.
This media query for max-width: 1300px appears to be incorrect. Because it is placed after the queries for 1024px and 435px, it overrides their styles on any screen smaller than 1300px. For instance, a 400px wide screen will receive a font-size of 1.1rem for .description instead of the intended 1rem from the max-width: 435px rule.
Furthermore, the styles within this block are identical to those in the @media (min-width: 1300px) and (max-width: 1515px) block, which suggests this might be a copy-paste error.
To resolve this, I recommend removing this entire block. For better long-term maintainability, it's also a good practice to order max-width media queries from the largest breakpoint to the smallest.
| @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; | ||
| } | ||
| } |
There was a problem hiding this comment.
The margin-left: auto; and margin-right: auto; rules in this block are being overridden by the margin: 0 0.25rem; rule within the @media (max-width: 768px) block at line 135. Since both media queries apply to screens between 565px and 768px, the rule that appears later in the file takes precedence, rendering your new margin rules ineffective.
To fix this, you should reorder or refactor the media queries to ensure the correct styles are applied. One approach is to consolidate all styles for a given breakpoint and ensure the more specific rules are not unintentionally overridden.
| @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 adjacent media queries contain identical style rules. To reduce code duplication and improve maintainability, they can be combined into a single, simpler media query.
| @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; | |
| } | |
| } | |
| } |
| .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; | ||
| } |
There was a problem hiding this comment.
Using !important can make your CSS harder to maintain and debug because it disrupts the natural cascading order of stylesheets. It's generally better to increase selector specificity to override styles. Please check if you can achieve the desired outcome by using more specific selectors instead of !important.
| .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; | ||
| } |
There was a problem hiding this comment.
Similar to the media query above, using !important should be avoided if possible as it can make styles difficult to override in the future. Consider using a more specific selector to enforce these styles, which will make the codebase easier to maintain.
2 participants
Output after the fixes
Benefit for the website
Before :
After :
Before:
After:
