-
Notifications
You must be signed in to change notification settings - Fork 0
Add section navigation to resume page #73
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
Conversation
WalkthroughResumePage.vue restructures the template to add an in-page navigation bar linking to anchored subsections for Education, Work experience, and Recommendations. Each subsection wraps its respective partial. Heading margin is adjusted, and scroll targets with scroll-mt classes are added to align content on navigation. Changes
Sequence Diagram(s)sequenceDiagram
actor U as User
participant B as Browser
participant R as ResumePage.vue
U->>B: Click nav link (#education / #work-experience / #recommendations)
B->>R: Navigate to anchor
note over B,R: Apply CSS scroll-margin-top (scroll-mt-*) for alignment
B-->>U: Scroll to targeted section
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gocanto, 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 enhances the user experience on the resume page by introducing a new navigation menu. This menu provides direct links to key sections such as education, work experience, and recommendations, allowing users to efficiently browse the content without manual scrolling. The implementation involves adding a navigation bar and assigning unique identifiers to each section for seamless in-page navigation. 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
|
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.
Code Review
This pull request adds in-page navigation to the resume page, which is a great enhancement for user experience. The implementation is straightforward, but I've identified a couple of areas for improvement. My main feedback is to make the navigation links and the content sections dynamic based on data availability. This will prevent showing links to empty sections and will make the page cleaner. I've provided specific code suggestions to implement this.
| <nav | ||
| class="flex flex-wrap items-center gap-3 text-sm font-medium text-slate-500 dark:text-slate-400 border border-slate-200 dark:border-slate-800 rounded-full px-4 py-2 mb-10" | ||
| aria-label="Resume sections" | ||
| > | ||
| <a | ||
| class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" | ||
| href="#education" | ||
| > | ||
| Education | ||
| </a> | ||
| <span class="text-slate-300 dark:text-slate-700">·</span> | ||
| <a | ||
| class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" | ||
| href="#work-experience" | ||
| > | ||
| Work experience | ||
| </a> | ||
| <span class="text-slate-300 dark:text-slate-700">·</span> | ||
| <a | ||
| class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" | ||
| href="#recommendations" | ||
| > | ||
| Recommendations | ||
| </a> | ||
| </nav> |
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.
The navigation links are currently static. This means that even if a section (like 'Recommendations') has no content, the link to it will still be displayed. This can lead to a confusing user experience where a user clicks a link and is scrolled to an empty part of the page.
To improve this, the navigation should be generated dynamically based on the presence of data for each section. I suggest creating a computed property, for example navLinks, which filters the sections that have content. Then, you can loop through this property to render the links.
Here is an example of what the computed property could look like in your <script> section:
import { computed } from 'vue';
const navLinks = computed(() => {
const links = [];
if (education.value && education.value.length > 0) {
links.push({ href: '#education', text: 'Education' });
}
if (experience.value && experience.value.length > 0) {
links.push({ href: '#work-experience', text: 'Work experience' });
}
if (recommendations.value && recommendations.value.length > 0) {
links.push({ href: '#recommendations', text: 'Recommendations' });
}
return links;
});With this computed property, you can simplify the template significantly and also hide the navigation bar completely if no sections have content.
<nav
v-if="navLinks.length"
class="flex flex-wrap items-center gap-3 text-sm font-medium text-slate-500 dark:text-slate-400 border border-slate-200 dark:border-slate-800 rounded-full px-4 py-2 mb-10"
aria-label="Resume sections"
>
<template v-for="(link, index) in navLinks" :key="link.href">
<a
class="transition-colors hover:text-slate-700 dark:hover:text-slate-200"
:href="link.href"
>
{{ link.text }}
</a>
<span v-if="index < navLinks.length - 1" class="text-slate-300 dark:text-slate-700">·</span>
</template>
</nav>
| <section id="education" class="scroll-mt-24"> | ||
| <EducationPartial v-if="education" :education="education" /> | ||
| </section> | ||
| <section id="work-experience" class="scroll-mt-24"> | ||
| <ExperiencePartial v-if="experience" :experience="experience" /> | ||
| </section> | ||
| <section id="recommendations" class="scroll-mt-24"> | ||
| <RecommendationPartial v-if="recommendations" :recommendations="recommendations" /> | ||
| </section> |
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.
Currently, the <section> wrappers are always rendered, even if there is no data for them. This can lead to empty sections in the DOM. For example, if education is null, an empty <section id="education"> is rendered. If education is an empty array, the EducationPartial component will render a heading but no content, which might not be the desired behavior.
To make the code cleaner and avoid rendering empty sections, it's better to conditionally render the entire <section> block. You can move the v-if directive to the <section> element and check if the data array is not empty. This also makes the v-if on the child component redundant.
This change, combined with the dynamic navigation links I suggested earlier, will ensure that links are only shown for sections that actually have content, and those sections are only rendered when they have data.
<section v-if="education && education.length" id="education" class="scroll-mt-24">
<EducationPartial :education="education" />
</section>
<section v-if="experience && experience.length" id="work-experience" class="scroll-mt-24">
<ExperiencePartial :experience="experience" />
</section>
<section v-if="recommendations && recommendations.length" id="recommendations" class="scroll-mt-24">
<RecommendationPartial :recommendations="recommendations" />
</section>
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: 1
🧹 Nitpick comments (3)
src/pages/ResumePage.vue (3)
30-30: Hide decorative separators from screen readers.Mark the middle-dot separators as decorative so they’re not announced.
- <span class="text-slate-300 dark:text-slate-700">·</span> + <span class="text-slate-300 dark:text-slate-700" aria-hidden="true">·</span>- <span class="text-slate-300 dark:text-slate-700">·</span> + <span class="text-slate-300 dark:text-slate-700" aria-hidden="true">·</span>Also applies to: 37-37
24-29: Add visible focus states for keyboard users on nav links.Improve accessibility by adding focus-visible styles.
- <a - class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" + <a + class="transition-colors hover:text-slate-700 dark:hover:text-slate-200 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-slate-400 dark:focus-visible:ring-slate-600 rounded-md" href="#education" >- <a - class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" + <a + class="transition-colors hover:text-slate-700 dark:hover:text-slate-200 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-slate-400 dark:focus-visible:ring-slate-600 rounded-md" href="#work-experience" >- <a - class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" + <a + class="transition-colors hover:text-slate-700 dark:hover:text-slate-200 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-slate-400 dark:focus-visible:ring-slate-600 rounded-md" href="#recommendations" >Also applies to: 31-36, 38-43
20-44: Render the nav from data to avoid links to empty sections.If a dataset is empty/null, consider hiding its nav item. Also removes manual separators.
Template refactor:
- <nav + <nav class="flex flex-wrap items-center gap-3 text-sm font-medium text-slate-500 dark:text-slate-400 border border-slate-200 dark:border-slate-800 rounded-full px-4 py-2 mb-10" aria-label="Resume sections" > - <a - class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" - href="#education" - > - Education - </a> - <span class="text-slate-300 dark:text-slate-700">·</span> - <a - class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" - href="#work-experience" - > - Work experience - </a> - <span class="text-slate-300 dark:text-slate-700">·</span> - <a - class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" - href="#recommendations" - > - Recommendations - </a> + <template v-for="(item, i) in navSections" :key="item.id"> + <a + class="transition-colors hover:text-slate-700 dark:hover:text-slate-200 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-slate-400 dark:focus-visible:ring-slate-600 rounded-md" + :href="`#${item.id}`" + > + {{ item.label }} + </a> + <span v-if="i < navSections.length - 1" class="text-slate-300 dark:text-slate-700" aria-hidden="true">·</span> + </template> </nav>Script additions (outside this hunk):
import { ref, onMounted, computed } from 'vue'; const navSections = computed(() => { const items: { id: string; label: string }[] = []; if (education.value?.length) items.push({ id: 'education', label: 'Education' }); if (experience.value?.length) items.push({ id: 'work-experience', label: 'Work experience' }); if (recommendations.value?.length) items.push({ id: 'recommendations', label: 'Recommendations' }); return items; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/ResumePage.vue(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test Format Workflow
src/pages/ResumePage.vue
[error] 1-1: Command 'git diff --exit-code' failed with exit code 1: detected changes in src/pages/ResumePage.vue (CI comparison reported differences).
🔇 Additional comments (1)
src/pages/ResumePage.vue (1)
48-56: Verify scroll offset across breakpoints.Ensure scroll-mt-24 matches the sticky header height on all breakpoints; adjust with responsive classes if needed (e.g., md:scroll-mt-28, lg:scroll-mt-32).
| <section> | ||
| <!-- Page title --> | ||
| <h1 class="h1 font-aspekta mb-6 md:mb-8">My resume</h1> | ||
|
|
||
| <nav | ||
| class="flex flex-wrap items-center gap-3 text-sm font-medium text-slate-500 dark:text-slate-400 border border-slate-200 dark:border-slate-800 rounded-full px-4 py-2 mb-10" | ||
| aria-label="Resume sections" | ||
| > | ||
| <a | ||
| class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" | ||
| href="#education" | ||
| > | ||
| Education | ||
| </a> | ||
| <span class="text-slate-300 dark:text-slate-700">·</span> | ||
| <a | ||
| class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" | ||
| href="#work-experience" | ||
| > | ||
| Work experience | ||
| </a> | ||
| <span class="text-slate-300 dark:text-slate-700">·</span> | ||
| <a | ||
| class="transition-colors hover:text-slate-700 dark:hover:text-slate-200" | ||
| href="#recommendations" | ||
| > | ||
| Recommendations | ||
| </a> | ||
| </nav> | ||
|
|
||
| <!-- Page content --> | ||
| <div class="text-slate-500 dark:text-slate-400 space-y-12"> | ||
| <section id="education" class="scroll-mt-24"> | ||
| <EducationPartial v-if="education" :education="education" /> | ||
| </section> | ||
| <section id="work-experience" class="scroll-mt-24"> | ||
| <ExperiencePartial v-if="experience" :experience="experience" /> | ||
| </section> | ||
| <section id="recommendations" class="scroll-mt-24"> | ||
| <RecommendationPartial v-if="recommendations" :recommendations="recommendations" /> | ||
| </section> | ||
| </div> | ||
| </section> |
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.
🧩 Analysis chain
Fix formatting to satisfy CI (Prettier/format check).
The “Test Format Workflow” is failing on this file. Run the repo’s formatter and commit the result so CI passes.
Try this script to auto-fix and show the diff:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Try project formatter if available
if command -v pnpm >/dev/null 2>&1; then
pnpm -s run format:check || pnpm -s run format || true
elif command -v npm >/dev/null 2>&1; then
npm run -s format:check || npm run -s format || true
elif command -v yarn >/dev/null 2>&1; then
yarn -s format:check || yarn -s format || true
fi
# Fallback to Prettier directly on the changed file
npx --yes prettier@3 --write "src/pages/ResumePage.vue" || true
# Show the resulting diff for this file
git diff -- src/pages/ResumePage.vueLength of output: 6461
Commit Prettier formatting updates to src/pages/ResumePage.vue
CI’s “Test Format Workflow” is failing; commit the diff from Prettier to satisfy the formatting check.
🤖 Prompt for AI Agents
In src/pages/ResumePage.vue around lines 16-58 the file needs Prettier
formatting applied and committed to satisfy CI’s "Test Format Workflow"; run
your project's Prettier/format command (e.g., npm/yarn format or npx prettier
--write) on this file (or the repo), review the changes, stage the resulting
formatted file, and create a new commit (with a message like "chore: run
Prettier on ResumePage.vue") so the CI formatting check passes.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68d6576321bc83338c964abf9b44edd4
Summary by CodeRabbit
New Features
Style