Filter steden tour (not done) #79
Conversation
WalkthroughThis pull request updates the project documentation and several Svelte components. The changelog now features a renamed "aanpassingen" section with entries detailing carousel improvements such as responsive design and interactive enhancements. In addition, a new CSS rule is added in the Header component to create a hover effect on navigation items. Finally, the TicketsOverview component gains a dynamic city selection feature that replaces a static list with reactive buttons, alongside updated styling adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Tickets as TicketsOverview
User->>Tickets: Click on a city button
Tickets->>Tickets: Invoke selectCity(city)
Tickets->>Tickets: Update selectedCity (null or chosen city)
Tickets->>UI: Re-render component with active button state
Possibly related PRs
🪧 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 wogo-agency ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
❌ Deploy Preview for wogo failed. Why did it fail? →
|
![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
🧹 Nitpick comments (4)
src/lib/components/pages/TicketsOverview.svelte (3)
9-11: Consider using English for code comments.To improve maintainability and collaboration, consider translating the Dutch comments to English.
-// Als de waarde van city gelijk is aan "All", dan wordt selectedCity ingesteld op null. -// Anders wordt selectedCity ingesteld op de waarde van city (de stad die is geselecteerd). +// If the city value is "All", selectedCity is set to null. +// Otherwise, selectedCity is set to the selected city value.
12-18: Simplify the selectCity function.The function can be simplified using a ternary operator.
-const selectCity = (city) => { - if (city === "All") { - selectedCity = null; - } else { - selectedCity = city; - } -}; +const selectCity = (city) => selectedCity = city === "All" ? null : city;
102-102: Improve cross-browser compatibility for hidden scrollbar.The current implementation only hides the scrollbar in WebKit browsers. Add cross-browser support.
overflow-x: auto; +scrollbar-width: none; /* Firefox */ +-ms-overflow-style: none; /* IE and Edge */ .tours-cards::-webkit-scrollbar { display: none; }Also applies to: 107-109
docs/Changelog.md (1)
15-29: Improve changelog formatting and consistency.
- Consolidate duplicate date entries
- Fix list indentation (should be 2 spaces)
- Consider using English for consistency
-## 4-2-2025 [Unreleased] - -### aanpassingen -- Carrousel interactief gemaakt. - - Dit heb ik gedaan door in het bestand TickedCarousel.svelte te werken. -**Wat heb ik aangepast?** --De js code, zoadat de knoppen werken -- De scroll width aan gepast zodat elke kaar 1 voor 1 kan worden gescrolld - -## 4-2-2025 [Unreleased] - -### aanpassingen -- filters active state gemaakt -- buttons van de filters gemaakt zodat ze tabbaar zijn +## 4-2-2025 [Unreleased] + +### Changes +- Made carousel interactive + - Updated TicketCarousel.svelte + - Implemented button functionality + - Adjusted scroll width for single card scrolling +- Added filter improvements + - Implemented active states for filters + - Made filter buttons keyboard accessible🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
19-19: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
24-24: Multiple headings with the same content
null(MD024, no-duplicate-heading)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/Changelog.md(1 hunks)src/lib/components/organisms/Header.svelte(1 hunks)src/lib/components/pages/TicketsOverview.svelte(3 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/Changelog.md
[misspelling] ~8-~8: Er is ook een Nederlandse term voor.
Context: ...released] ### aanpassingen - Carrousel responsive gemaakt. - Dit heb ik gedaan door ...
(NL_SIMPLE_REPLACE_RESPONSIVE)
🪛 markdownlint-cli2 (0.17.2)
docs/Changelog.md
9-9: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
24-24: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
🔇 Additional comments (2)
src/lib/components/pages/TicketsOverview.svelte (1)
38-40:❓ Verification inconclusive
Verify city filtering implementation.
The TicketCard component receives itemCollection but doesn't appear to filter based on selectedCity.
🏁 Script executed:
#!/bin/bash # Search for filtering logic in TicketCard component ast-grep --pattern 'export let itemCollection'Length of output: 48
Missing city filtering logic in TicketCard
It appears that theTicketCardcomponent is being passeditemCollectionwithout any filtering based onselectedCity. Please verify whether:
- City filtering should be performed within the
TicketCardcomponent.- Or the filtering is intended to occur upstream (e.g., in the parent component) before
itemCollectionis passed.If filtering is meant to be implemented here, please add the logic to filter
itemCollectionbased on the currentselectedCity. Otherwise, please clarify the data flow so that the intended behavior is evident.src/lib/components/organisms/Header.svelte (1)
206-214: LGTM!The hover effect is well-implemented, using consistent design tokens and smooth transitions.
| <ul> | ||
| <li>Amsterdam</li> | ||
| <li>Rotterdam</li> | ||
| <li>Utrecht</li> | ||
| <button class:active={!selectedCity} | ||
| on:click={() => selectCity("All")}> | ||
| All</button> <!-- selected city word null --> | ||
| {#each citiesList as city} | ||
| <button class:active={selectedCity === city} | ||
| on:click={() => selectCity(city)} | ||
| tabindex="0"> | ||
| {city} | ||
| </button> | ||
| {/each} | ||
| </ul> |
There was a problem hiding this comment.
Fix list structure and improve accessibility.
The buttons are placed directly inside a <ul> element, which is invalid HTML. Wrap them in <li> elements for proper list structure.
<ul>
- <button class:active={!selectedCity}
- on:click={() => selectCity("All")}>
- All</button>
+ <li>
+ <button class:active={!selectedCity}
+ on:click={() => selectCity("All")}>
+ All
+ </button>
+ </li>
{#each citiesList as city}
- <button class:active={selectedCity === city}
- on:click={() => selectCity(city)}
- tabindex="0">
- {city}
- </button>
+ <li>
+ <button class:active={selectedCity === city}
+ on:click={() => selectCity(city)}>
+ {city}
+ </button>
+ </li>
{/each}
</ul>Note: Remove the explicit tabindex="0" as buttons are naturally focusable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ul> | |
| <li>Amsterdam</li> | |
| <li>Rotterdam</li> | |
| <li>Utrecht</li> | |
| <button class:active={!selectedCity} | |
| on:click={() => selectCity("All")}> | |
| All</button> <!-- selected city word null --> | |
| {#each citiesList as city} | |
| <button class:active={selectedCity === city} | |
| on:click={() => selectCity(city)} | |
| tabindex="0"> | |
| {city} | |
| </button> | |
| {/each} | |
| </ul> | |
| <ul> | |
| <li> | |
| <button class:active={!selectedCity} | |
| on:click={() => selectCity("All")}> | |
| All | |
| </button> | |
| </li> | |
| {#each citiesList as city} | |
| <li> | |
| <button class:active={selectedCity === city} | |
| on:click={() => selectCity(city)}> | |
| {city} | |
| </button> | |
| </li> | |
| {/each} | |
| </ul> |
What does this change?
This pull request will add a filter function. You can filter the cards by city
Resolves issue #74 in #59
How Has This Been Tested?
Images
before:
after :
The issues have a hover and click preview
How to review
src/lib/components/pages/TicketsOverview.svelte:
Summary by CodeRabbit