-
-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Overall UI Improvement in website #963
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe pull request includes a comprehensive redesign of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
index.html
(1 hunks)new.css
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- new.css
🔇 Additional comments (1)
index.html (1)
648-648
:
Correct the Font Awesome class for the Twitter icon
The class fa-x-twitter
at line 648 is invalid. The correct class for the Twitter icon in Font Awesome is fa-twitter
. Updating this ensures the icon displays correctly.
Apply this diff to fix the class name:
Likely invalid or redundant comment.
<div class="cards"> | ||
<!-- Our Mission Card --> | ||
<div class="card"> | ||
<img src="images/Gemini_Generated_Image_fx9dejfx9dejfx9d.jpg" alt=""> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add descriptive alt text to images for accessibility
The <img>
elements at lines 563, 576, and 593 have empty alt
attributes. Providing descriptive alt
text improves accessibility for users relying on screen readers and enhances SEO.
Apply this diff to add descriptive alt text:
+ <img src="images/Gemini_Generated_Image_fx9dejfx9dejfx9d.jpg" alt="Illustration representing our mission">
+ <img src="images/Gemini_Generated_Image_52rg8r52rg8r52rg.jpg" alt="Illustration explaining how it works">
+ <img src="images/Gemini_Generated_Image_ootrdvootrdvootr.jpg" alt="Illustration showing why to choose us">
Also applies to: 576-576, 593-593
.footer a { | ||
color: var(--soft-gold); | ||
text-decoration: none; | ||
font-size: 16px; | ||
} | ||
|
||
.testimonial-card img { | ||
width: 80px; | ||
/* Profile image width */ | ||
height: 80px; | ||
/* Profile image height */ | ||
border-radius: 50%; | ||
/* Circular shape for profile images */ | ||
margin-bottom: 20px; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve conflicting CSS styles for .footer a
selector
The .footer a
selector is defined with different color
properties at lines 184 (var(--soft-gold)
) and 679 (white
). This conflict can lead to unexpected styling. Consolidate these styles to maintain consistency in link colors within the footer.
Also applies to: 678-680
<br> | ||
<br> | ||
<br> | ||
<br> | ||
<br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid using multiple <br>
tags for spacing; use CSS margins instead
Using multiple <br>
tags (lines 546-550) for spacing is not a best practice and may result in inconsistent spacing across devices. Utilize CSS margin or padding properties to control spacing between elements.
Apply this diff to remove the <br>
tags:
And adjust spacing with CSS, for example:
.panel-1 {
padding-top: 100px; /* Adjust the value as needed */
}
top: 0; | ||
left: 0; | ||
z-index: 9999; | ||
} | ||
|
||
#progress-bar { | ||
height: 100%; | ||
background-color: #3f10ea; | ||
} | ||
|
||
.panel-1 { | ||
padding-top: 40px; | ||
width: 100vw; | ||
height: auto; | ||
position: relative; | ||
} | ||
|
||
.about { | ||
width: 100%; | ||
display: flex; | ||
flex-direction: column; | ||
justify-self: center; | ||
align-items: center; | ||
justify-content: center; | ||
cursor: pointer; | ||
box-shadow: 0 2px 10px rgba(0, 0, 0, 0.2); | ||
transition: background-color 0.3s, transform 0.3s; | ||
} | ||
text-align: center; | ||
padding-top: 5rem; | ||
padding-bottom: 5rem; | ||
} | ||
|
||
.about h1 { | ||
font-size: 4rem; | ||
margin-bottom: 1rem; | ||
font-weight: 600; | ||
} | ||
|
||
.about p { | ||
width: 70%; | ||
font-size: 1.8rem; | ||
} | ||
|
||
.cards { | ||
width: 100%; | ||
display: flex; | ||
justify-content: space-evenly; | ||
position: relative; | ||
padding-bottom: 5rem; | ||
} | ||
|
||
.card { | ||
width: 30%; | ||
padding: 2rem; | ||
border-radius: 2rem; | ||
background-color: #f8f9fa; | ||
} | ||
|
||
.card:hover { | ||
scale: 105%; | ||
} | ||
|
||
.card h2 { | ||
font-size: 2.8rem; | ||
margin-bottom: 1rem; | ||
font-weight: 600; | ||
} | ||
|
||
.card p { | ||
font-size: 1.5rem; | ||
margin-bottom: 1rem; | ||
} | ||
|
||
.card ul li { | ||
font-size: 1.5rem; | ||
} | ||
|
||
.card img { | ||
margin-bottom: 2rem; | ||
} | ||
|
||
@media (max-width:450px) { | ||
.cards { | ||
flex-direction: column; | ||
justify-content: center; | ||
align-items: center; | ||
gap: 3rem; | ||
} | ||
|
||
.scroll-to-top:hover { | ||
background-color: #005580; | ||
transform: scale(1.1); | ||
} | ||
.card { | ||
width: 90%; | ||
} | ||
} | ||
</style> | ||
</head> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider moving inline CSS to an external stylesheet
The inline CSS within the <style>
tag spans lines 18-444, making the HTML file large and harder to manage. Moving these styles to an external stylesheet (e.g., new.css
) can enhance maintainability, enable browser caching, and reduce page load time.
const toggler = document.getElementById("toggler"); | ||
const body = document.body; | ||
const closeBtn = document.querySelector(".close-btn"); | ||
|
||
closeBtn.addEventListener("click", function () { | ||
toggler.checked = false; | ||
body.classList.remove("sidebar-active"); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure the 'close-btn' element exists before adding an event listener
At lines 688-693, an event listener is added to closeBtn
, which is selected using document.querySelector(".close-btn")
. Since there is no element with the class close-btn
in the HTML, closeBtn
will be null
, causing an error. Remove the unused code or add the missing element.
Apply this diff to remove the unused code:
Committable suggestion was skipped due to low confidence.
/* .navbar { | ||
background-color: var(--deep-teal); /* Updated navbar background color | ||
padding: 31px 20px; /* Increased top and bottom padding | ||
display: flex; | ||
justify-content: space-between; | ||
align-items: center; | ||
} | ||
|
||
.navbar img{ | ||
width: 100px; | ||
padding: 0px 6px; | ||
margin-right: -64px; | ||
} | ||
|
||
.navbar h1 { | ||
color: var(--off-white); /* Changed to use off-white for better contrast | ||
margin: 0; | ||
font-size: 2rem; | ||
margin-left: -38rem; /* Adjust this value to move the logo right | ||
transition: color 0.3s ease; /* Smooth transition for color change | ||
} | ||
|
||
.navbar h1:hover { | ||
color: var(--soft-gold); /* Color change on hover | ||
} | ||
|
||
.navbar ul { | ||
list-style-type: none; | ||
margin: 0; | ||
padding: 0; | ||
display: flex; | ||
gap: 20px; | ||
margin-right: 30px; /* Adjust this value to move the items left | ||
} | ||
|
||
.navbar ul li { | ||
display: inline; | ||
} | ||
|
||
.navbar ul li a { | ||
color: var(--off-white); /* Updated navbar link color * | ||
text-decoration: none; | ||
font-size: 18px; | ||
} | ||
|
||
.navbar ul li a:hover { | ||
color: var(--soft-gold); /* Updated hover color to Soft Gold * | ||
text-decoration: none; | ||
transition: color 0.3s ease; | ||
} */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid nested CSS comments to prevent syntax errors
The comment block starting at line 235 contains nested /* */
comments, which is not allowed in CSS and may lead to syntax errors. Adjust the comments to prevent nesting or remove redundant comments.
Apply this diff to fix the nested comments:
+ /* .navbar {
+ background-color: var(--deep-teal); /* Updated navbar background color */
+ padding: 31px 20px; /* Increased top and bottom padding */
Committable suggestion was skipped due to low confidence.
Solved issue number #962
Dear mam, please accept this pull request and close issue number #962 as i resolved it, and please accept and review my code.
Summary by CodeRabbit
New Features
Styling Enhancements
new.css
).