Style, Refactor, Feature updates/fixes #79
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
components of SVG's
fixes #78 Made a Prop with a class in the SVG of the component, because of this there weren't 2 SVG's needed. Both were Arrows, 1 to the left and 1 to the right. I made a prop which gives 1 the class right-arrow with transformX -1. This makes the left arrow turn to the right.
WalkthroughThis pull request introduces two new Svelte components—Arrow and HvaLogo—for rendering SVG icons. The new Arrow component replaces inline SVGs in the Pagination component, while HvaLogo is integrated into the Nav component. In addition, numerous Svelte files (atoms, molecules, organisms, and route files) have been updated with formatting improvements, consistent semicolon usage, and minor structural adjustments. These changes enhance code readability and consistency without altering core functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant P as Pagination Component
participant A as Arrow Component
U->>P: Request navigation update
P->>A: Render arrow icon (set rightArrow prop if needed)
A-->>P: Return SVG representation
P-->>U: Updated pagination display
sequenceDiagram
participant U as User
participant N as Navigation Component
participant H as HvaLogo Component
U->>N: Load navigation
N->>H: Render logo component
H-->>N: Return responsive SVG logo
N-->>U: Display complete navigation
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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 (
|
❌ Deploy Preview for programma-agency failed.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/lib/organisms/Semesters.svelte (1)
172-191: 🛠️ Refactor suggestionRemove commented-out code.
Clean up the codebase by removing unused commented code for gradient overlays.
- /* .semester-grid::before, .semester-grid::after { - content: ''; - position: absolute; - top: 0; - bottom: 0; - width: 10px; - pointer-events: none; - } - - .semester-grid::before { - left: 0; - background: linear-gradient(to right, var(--grey), rgba(255, 255, 255, 0)); - z-index: 1; - } - - .semester-grid::after { - right: 0; - background: linear-gradient(to left, var(--grey), rgba(255, 255, 255, 0)); - z-index: 1; - } */
🧹 Nitpick comments (11)
src/lib/molecules/SprintLink.svelte (2)
14-14: Use strict equality operator for number comparison.Replace
==with===for number comparison as it's more type-safe.- let semester4 = sprint.sprintNumber == 19 || sprint.sprintNumber == 20; + let semester4 = sprint.sprintNumber === 19 || sprint.sprintNumber === 20;
90-92: Consider using a CSS custom property for semester4 height.The hardcoded height value could be moved to a CSS custom property for better maintainability.
- li.semester4 { - height: 222px; - } + :root { + --semester4-height: 222px; + } + + li.semester4 { + height: var(--semester4-height); + }src/lib/atoms/Arrow.svelte (1)
5-21: Consider adding a title attribute for better accessibility.While the SVG implementation is clean and includes good accessibility attributes, adding a title element would improve screen reader support.
<svg class={rightArrow} aria-hidden="true" role="img" xmlns="http://www.w3.org/2000/svg" width="20" height="20" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" > + <title>Navigation Arrow</title> <path stroke="none" d="M0 0h24v24H0z" fill="none" /><path d="M5 12l14 0" /><path d="M5 12l4 4" /><path d="M5 12l4 -4" /> </svg>src/lib/molecules/Pagination.svelte (2)
1-4: Consider reordering imports before props.While the code works, it's a common convention to place imports before props declarations.
<script> + import Arrow from "../atoms/Arrow.svelte"; let { prevSprint, nextSprint } = $props(); - import Arrow from "../atoms/Arrow.svelte"; </script>
32-73: Consider adding transition for color changes.The hover/focus styles could benefit from smooth transitions for better user experience.
nav { font-size: 1rem; font-weight: 600; display: flex; justify-content: space-between; margin: 2rem 0 0; a { text-decoration: none; color: var(--blueberry); display: flex; align-items: center; gap: 0.25rem; cursor: pointer; + transition: color 0.2s ease-in-out;src/routes/[semester]/[sprint]/+page.server.js (1)
75-92: Optimize array operations in formatTasks.Consider using chaining for better readability and performance.
-function formatTasks({ search: { repos } }) { - return repos - .filter((repo) => repo != null) - .map(({ repo }) => { - const topics = repo.repositoryTopics.edges - .map((topic) => topic.node.topic.name) - .filter((topic) => topic == "task" || topic == "subtask"); +function formatTasks({ search: { repos } }) { + return repos + ?.filter((repo) => repo != null) + ?.map(({ repo }) => ({ + name: formatName(repo.name), + description: repo.description, + url: repo.url, + forkCount: repo.forkCount, + forks: formatForks(repo), + topic: repo.repositoryTopics.edges + .map((topic) => topic.node.topic.name) + .find((topic) => topic === "task" || topic === "subtask"), + })) ?? []; +}src/lib/organisms/Semesters.svelte (1)
48-52: Consider using CSS transform for better animation performance.The animation could benefit from using transform properties for better performance.
@keyframes waka_waka_waka { to { - transform: translate(-50%, var(--translation)) rotate(var(--rotation)); + transform: translate3d(-50%, var(--translation), 0) rotate(var(--rotation)); } }Also applies to: 193-198
src/lib/atoms/HvaLogo.svelte (2)
1-13: Enhance SVG accessibility.While the SVG includes a title, it could benefit from additional accessibility attributes.
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 250 35" title="Hva logo" class="hva-logo" + role="img" + aria-labelledby="hva-logo-title" > + <title id="hva-logo-title">Hva logo</title> <path
23-40: Simplify media query syntax.The media query syntax can be simplified for better maintainability.
- @media (760px <= width) { + @media (min-width: 760px) { .hva-logo { width: 40%; } } - @media (1025px <= width) { + @media (min-width: 1025px) { .hva-logo { height: auto; width: 20%; } } - @media (1600 <= width) { + @media (min-width: 1600px) { .hva-logo { width: 15%; } }src/lib/organisms/Nav.svelte (2)
219-284: Optimize media queries and animations.
- Simplify media query syntax for consistency.
- Consider using
transforminstead of manipulatingrightandtopproperties for better performance.- @media (760px <= width) { + @media (min-width: 760px) { header { .top { // ... } } } - @media (1025px <= width) { + @media (min-width: 1025px) { header { // ... } } .fdnd-logo:focus-visible::before, .fdnd-logo:hover::before { - right: 1px; - top: 1px; + transform: translate(-2px, -1px); }
288-292: Consider using transform for animation.The
upanimation would perform better using transform.@keyframes up { to { - transform: translateY(0); + transform: translate3d(0, 0, 0); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/lib/atoms/Arrow.svelte(1 hunks)src/lib/atoms/HvaLogo.svelte(1 hunks)src/lib/molecules/Heading.svelte(1 hunks)src/lib/molecules/Pagination.svelte(1 hunks)src/lib/molecules/Program.svelte(1 hunks)src/lib/molecules/Semester.svelte(5 hunks)src/lib/molecules/SprintLink.svelte(5 hunks)src/lib/molecules/StudentWork.svelte(3 hunks)src/lib/molecules/Topics.svelte(2 hunks)src/lib/organisms/ContentSemester.svelte(2 hunks)src/lib/organisms/ContentSprint.svelte(2 hunks)src/lib/organisms/Footer.svelte(6 hunks)src/lib/organisms/Goal.svelte(1 hunks)src/lib/organisms/Nav.svelte(6 hunks)src/lib/organisms/Schedule.svelte(5 hunks)src/lib/organisms/Semesters.svelte(2 hunks)src/lib/organisms/Sprints.svelte(2 hunks)src/lib/organisms/Tasks.svelte(5 hunks)src/routes/+layout.svelte(2 hunks)src/routes/+page.server.js(1 hunks)src/routes/+page.svelte(1 hunks)src/routes/[semester]/+page.server.js(1 hunks)src/routes/[semester]/+page.svelte(2 hunks)src/routes/[semester]/[sprint]/+page.server.js(1 hunks)src/routes/[semester]/[sprint]/+page.svelte(2 hunks)src/routes/api/[semester]/+server.js(1 hunks)src/routes/api/[semester]/[sprint]/+server.js(1 hunks)
✅ Files skipped from review due to trivial changes (20)
- src/routes/api/[semester]/[sprint]/+server.js
- src/routes/[semester]/[sprint]/+page.svelte
- src/routes/+page.svelte
- src/lib/organisms/Sprints.svelte
- src/routes/[semester]/+page.server.js
- src/routes/+page.server.js
- src/lib/molecules/Program.svelte
- src/lib/organisms/Footer.svelte
- src/lib/organisms/ContentSemester.svelte
- src/lib/molecules/Topics.svelte
- src/routes/api/[semester]/+server.js
- src/lib/organisms/Goal.svelte
- src/lib/molecules/StudentWork.svelte
- src/lib/molecules/Heading.svelte
- src/lib/molecules/Semester.svelte
- src/routes/+layout.svelte
- src/lib/organisms/Tasks.svelte
- src/lib/organisms/Schedule.svelte
- src/lib/organisms/ContentSprint.svelte
- src/routes/[semester]/+page.svelte
🔇 Additional comments (5)
src/lib/molecules/SprintLink.svelte (1)
22-45: LGTM! Clean and semantic template structure.The template changes look good. The class binding using array join is a clean approach, and the markup is semantic with proper accessibility attributes.
src/lib/atoms/Arrow.svelte (2)
1-3: LGTM! Clean props implementation.The component effectively uses the new $props() syntax for prop destructuring.
23-31: LGTM! Efficient CSS transformation.The CSS implementation for arrow direction is clean and efficient, using scaleX transformation.
src/lib/molecules/Pagination.svelte (1)
6-30: LGTM! Clean navigation implementation.The navigation structure is semantic and accessible, with good use of the Arrow component.
src/lib/organisms/Nav.svelte (1)
2-3: LGTM! Clean component integration.The HvaLogo component is properly imported and integrated, aligning with the PR objective of converting SVGs into components.
Comment on lines
+8
to
+43
| export const load = async ({ params: { sprint } }) => { | ||
| prefix = sprint; | ||
|
|
||
| const querySprint = getQuerySprint(sprint); | ||
| const queryTasks = getQueryTasks(sprint); | ||
|
|
||
| const dataSprint = await client({ | ||
| query: querySprint, | ||
| variables: { slug: sprint }, | ||
| fetch: fetch, | ||
| endpoint: import.meta.env.VITE_HYPGRAPH_ENDPOINT, | ||
| }); | ||
| const dataTasks = await client({ | ||
| query: queryTasks, | ||
| variables: { slug: sprint }, | ||
| fetch: fetch, | ||
| endpoint: import.meta.env.VITE_GITHUB_ENDPOINT, | ||
| headers: headersGitHub, | ||
| }); | ||
|
|
||
| const { | ||
| sprint: { sprintNumber }, | ||
| } = dataSprint; | ||
|
|
||
| const { prevSprint, nextSprint } = await getSprintNavigation(sprintNumber); | ||
|
|
||
| dataSprint.sprint.prevSprint = prevSprint; | ||
| dataSprint.sprint.nextSprint = nextSprint; | ||
|
|
||
| if (dataTasks) { | ||
| const tasks = formatTasks(dataTasks, sprint); | ||
| return { ...dataSprint.sprint, tasks: tasks }; | ||
| } | ||
|
|
||
| return { ...dataSprint.sprint }; | ||
| }; |
There was a problem hiding this comment.
Add error handling for API calls.
The load function should handle potential API failures gracefully.
export const load = async ({ params: { sprint } }) => {
prefix = sprint;
const querySprint = getQuerySprint(sprint);
const queryTasks = getQueryTasks(sprint);
- const dataSprint = await client({
- query: querySprint,
- variables: { slug: sprint },
- fetch: fetch,
- endpoint: import.meta.env.VITE_HYPGRAPH_ENDPOINT,
- });
+ try {
+ const dataSprint = await client({
+ query: querySprint,
+ variables: { slug: sprint },
+ fetch: fetch,
+ endpoint: import.meta.env.VITE_HYPGRAPH_ENDPOINT,
+ });
+ // ... rest of the code
+ } catch (error) {
+ console.error('Failed to fetch sprint data:', error);
+ throw error;
+ }Committable suggestion skipped: line range outside the PR's diff.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change?
Resolves issue #62 , #73 , #76 , #77 , #78
#62 Expand Semester 4 Sprints
I made the Semester 4 Sprints bigger, with JS logic. I made them bigger, because the sprints are not just 3 weeks like other sprints. I've made a variable and put sprint 19 and 20 in it. I used a
.join(' ')to add a class to the sprints. Only the 19th and 20th sprint had a adjustment to their height, so another class than the others.#73 Aligned text with expand agenda button
I've removed the flex-direction column in the class of the label, because of this the text was next to the button. Also added align-text center so the text was aligned in the center.
#76 Refactored Code
I've refactored every file to fix the consistency in the code and unnecessary things, for example I deleted commented code and fixed ugly code like:
display:flex;(correct is:display: flex;)#77 Components for SVG
I deleted every SVG in a file and made components for them. I copied the SVG and the styling and dropped it in the SVG. This ensures that you have good overview in your code.
#78 2 Same components
There was 2 SVG's (An arrow) 1 pointing to the left and 1 pointing to the right. I deleted the right one and just kept the left one. I gave the arrow svg a prop so i could manipulate the css for it for the right one. I've added the class .right-arrow that has:
transform: scaleX(-1);which makes the arrow looking to the right direction, because of this you could just use 1 SVG instead of 2.How Has This Been Tested?
Accessibility & Performance test are done by google's lighthouse test.
Responsive test i've done my self the inspect in google and put the layout on responsive and slide it left and right to see if the site breaks and it the results were good.
Images
Accessibility test & Performance Test
Before & After of Semester 4
How to review
Summary by CodeRabbit
New Features
Style