Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/components/Hamburger.astro
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
import '../styles/global.css';
---

<div class="hamburger">
<span class="line"></span>
<span class="line"></span>
<span class="line"></span>
</div>
Comment on lines +6 to +10

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

For accessibility, interactive elements like a menu toggle should be a <button> instead of a <div>. This ensures it's keyboard-focusable and properly announced by screen readers. You should also include an aria-label for context and aria-expanded to indicate the menu's state (which should be toggled with JavaScript). Adding type="button" is also a good practice to prevent default form submission behavior.

<button class="hamburger" type="button" aria-label="Open menu" aria-expanded="false">
  <span class="line"></span>
  <span class="line"></span>
  <span class="line"></span>
</button>

13 changes: 13 additions & 0 deletions src/components/Header.astro
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
import '../styles/global.css';
import Hamburger from './Hamburger.astro';
import Navigation from './Navigation.astro';

---

<header>
<nav>
<Hamburger />
<Navigation />
</nav>
</header>
8 changes: 5 additions & 3 deletions src/components/Navigation.astro
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import '../styles/global.css';

---

<a href="/">Home</a>
<a href="/about/">About</a>
<a href="/blog/">Blog</a>
<div class="nav-links">
<a href="/">Home</a>
<a href="/about/">About</a>
<a href="/blog/">Blog</a>
</div>
2 changes: 1 addition & 1 deletion src/components/Social.astro
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import '../styles/global.css';
const { platform, username } = Astro.props;
---

<a href={`https://www.${platform}.com/${username}`}>{platform}</a>
<a href={`https://www.${platform}.com/${username}`} target='_blank'>{platform}</a>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using target="_blank" without rel="noopener noreferrer" creates a security vulnerability. The linked page can gain access to the originating page's window object via window.opener, which can be malicious. Adding rel="noopener noreferrer" prevents this.

<a href={`https://www.${platform}.com/${username}`} target="_blank" rel="noopener noreferrer">{platform}</a>


<style>
a {
Expand Down
7 changes: 5 additions & 2 deletions src/pages/about.astro
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
import '../styles/global.css';
import Navigation from '../components/Navigation.astro';
import Header from '../components/Header.astro';
import Footer from '../components/Footer.astro';

const pageTitle = "About Me";
Expand Down Expand Up @@ -44,7 +44,7 @@ const textCase = "uppercase"
</style>
</head>
<body>
<Navigation />
<Header />
<h1>{pageTitle}</h1>
<h2>... and my new Astro site!</h2>

Expand All @@ -71,5 +71,8 @@ const textCase = "uppercase"

{goal === 3 ? <p>My goal is to finish in 3 days.</p> : <p>My goal is not 3 days.</p>}
<Footer />
<script>
import '../scripts/menu.js';
</script>
Comment on lines +74 to +76

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The menu script is currently imported on each page individually. To improve maintainability and adhere to the Don't Repeat Yourself (DRY) principle, consider creating a shared Layout.astro component. This layout could encapsulate common elements like the <Header>, <Footer>, and this <script> tag, centralizing logic and reducing code duplication across your pages.

</body>
</html>
7 changes: 5 additions & 2 deletions src/pages/blog.astro
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
import '../styles/global.css';
import Navigation from '../components/Navigation.astro';
import Header from '../components/Header.astro';
import Footer from '../components/Footer.astro';

---
Expand All @@ -14,7 +14,7 @@ import Footer from '../components/Footer.astro';
<title>Astro</title>
</head>
<body>
<Navigation />
<Header />
<h1>My Astro Learning Blog</h1>
<p>This is where I will post about my journey learning Astro.</p>
<ul>
Expand All @@ -23,5 +23,8 @@ import Footer from '../components/Footer.astro';
<li><a href="/posts/post-3/">Post 3</a></li>
</ul>
<Footer />
<script>
import '../scripts/menu.js';
</script>
</body>
</html>
7 changes: 5 additions & 2 deletions src/pages/index.astro
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
import '../styles/global.css';
import Navigation from '../components/Navigation.astro';
import Header from '../components/Header.astro';
import Footer from '../components/Footer.astro';

const pageTitle = 'Home Page';
Expand All @@ -15,8 +15,11 @@ const pageTitle = 'Home Page';
<title>{pageTitle}</title>
</head>
<body>
<Navigation />
<Header />
<h1>{pageTitle}</h1>
<Footer />
<script>
import '../scripts/menu.js';
</script>
</body>
</html>
3 changes: 3 additions & 0 deletions src/scripts/menu.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
document.querySelector('.hamburger').addEventListener('click', () => {
document.querySelector('.nav-links').classList.toggle('expanded');
});
Comment on lines +1 to +3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This script could be more robust and accessible.

  1. Robustness: If .hamburger or .nav-links elements are not present on a page where this script runs, querySelector will return null, causing a TypeError. It's safer to check if the elements exist before adding event listeners.
  2. Accessibility: The state of the menu (expanded/collapsed) should be communicated to assistive technologies. You can do this by toggling the aria-expanded attribute on the hamburger button.
Suggested change
document.querySelector('.hamburger').addEventListener('click', () => {
document.querySelector('.nav-links').classList.toggle('expanded');
});
const hamburger = document.querySelector('.hamburger');
const navLinks = document.querySelector('.nav-links');
if (hamburger && navLinks) {
hamburger.addEventListener('click', () => {
navLinks.classList.toggle('expanded');
const isExpanded = navLinks.classList.contains('expanded');
hamburger.setAttribute('aria-expanded', isExpanded);
});
}

63 changes: 63 additions & 0 deletions src/styles/global.css
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,67 @@ body {
h1 {
margin: 1rem 0;
font-size: 2.5rem;
}

/* nav styles */

.hamburger {
padding-right: 20px;
cursor: pointer;
}

.hamburger .line {
display: block;
width: 40px;
height: 5px;
margin-bottom: 10px;
background-color: #ff9776;
}

.nav-links {
width: 100%;
top: 5rem;
left: 48px;
background-color: #ff9776;
display: none;
margin: 0;
}

.nav-links a {
display: block;
text-align: center;
padding: 10px 0;
text-decoration: none;
font-size: 1.2rem;
font-weight: bold;
text-transform: uppercase;
}

.nav-links a:hover,
.nav-links a:focus {
background-color: #ff9776;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The :hover and :focus states for navigation links share the same background color as the navigation container (#ff9776). This results in no visual feedback upon user interaction, which is detrimental to usability. It's recommended to use a distinct color to signify these states.

Suggested change
background-color: #ff9776;
background-color: #e6866a;

}

.expanded {
display: unset;
}

@media screen and (min-width: 636px) {
.nav-links {
margin-left: 5em;
display: block;
position: static;
width: auto;
background: none;
}

.nav-links a {
display: inline-block;
padding: 15px 20px;
}

.hamburger {
display: none;
}

}
5 changes: 4 additions & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
{
"extends": "astro/tsconfigs/strict",
"include": [".astro/types.d.ts", "**/*"],
"exclude": ["dist"]
"exclude": ["dist"],
"compilerOptions": {
"lib": ["esnext", "dom"]
}
}