-
Notifications
You must be signed in to change notification settings - Fork 1.9k
add task solution #1799
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
base: master
Are you sure you want to change the base?
add task solution #1799
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| import { NavLink } from 'react-router-dom'; | ||
| import classNames from 'classnames'; | ||
|
|
||
| export const Navbar = () => { | ||
| return ( | ||
| <nav | ||
|
|
@@ -8,17 +11,28 @@ export const Navbar = () => { | |
| > | ||
| <div className="container"> | ||
| <div className="navbar-brand"> | ||
| <a className="navbar-item" href="#/"> | ||
| <NavLink | ||
| to="/" | ||
| className={({ isActive }) => | ||
| classNames('navbar-item', { | ||
| 'has-background-grey-lighter': isActive, | ||
| }) | ||
| } | ||
|
Comment on lines
+17
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The link uses a hardcoded path |
||
| > | ||
| Home | ||
| </a> | ||
| </NavLink> | ||
|
|
||
| <a | ||
| <NavLink | ||
| aria-current="page" | ||
| className="navbar-item has-background-grey-lighter" | ||
| href="#/people" | ||
| to="/people" | ||
| className={({ isActive }) => | ||
| classNames('navbar-item', { | ||
| 'has-background-grey-lighter': isActive, | ||
| }) | ||
| } | ||
| > | ||
| People | ||
| </a> | ||
| </NavLink> | ||
| </div> | ||
| </div> | ||
| </nav> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,89 @@ | ||
| import { useSearchParams } from 'react-router-dom'; | ||
|
|
||
| export const PeopleFilters = () => { | ||
| const [searchParams, setSearchParams] = useSearchParams(); | ||
| const selectedCenturies = searchParams.getAll('centuries'); | ||
|
|
||
| const sex = searchParams.get('sex') || 'all'; | ||
|
|
||
| const setSex = (value: string) => { | ||
| const params = new URLSearchParams(searchParams); | ||
|
|
||
| if (value === 'all') { | ||
| params.delete('sex'); | ||
| } else { | ||
| params.set('sex', value); | ||
| } | ||
|
|
||
| setSearchParams(params); | ||
| }; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This navigation doesn't preserve search params. Per requirement #2, clicking on a person should keep the current filters/sort active. Use the SearchLink component or construct a link with the current search params included. |
||
|
|
||
| const handleQueryChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const params = new URLSearchParams(searchParams); | ||
|
|
||
| const value = e.target.value; | ||
|
|
||
| if (value) { | ||
| params.set('query', value); | ||
| } else { | ||
| params.delete('query'); | ||
| } | ||
|
|
||
| setSearchParams(params); | ||
| }; | ||
|
|
||
| const toggleCentury = (value: number) => { | ||
|
Comment on lines
+29
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical bug: When |
||
| const params = new URLSearchParams(searchParams); | ||
| const current = params.getAll('centuries'); | ||
| const strValue = String(value); | ||
|
|
||
| if (current.includes(strValue)) { | ||
| const updated = current.filter(c => c !== strValue); | ||
|
|
||
| params.delete('centuries'); | ||
| updated.forEach(c => params.append('centuries', c)); | ||
| } else { | ||
| params.append('centuries', strValue); | ||
| } | ||
|
|
||
| setSearchParams(params); | ||
| }; | ||
|
|
||
| const isSelected = (value: number) => | ||
| selectedCenturies.includes(String(value)); | ||
|
|
||
| const resetCenturies = () => { | ||
| const params = new URLSearchParams(searchParams); | ||
|
|
||
| params.delete('centuries'); | ||
| setSearchParams(params); | ||
| }; | ||
|
|
||
| const resetAllFilters = () => { | ||
| setSearchParams({}); | ||
| }; | ||
|
|
||
| return ( | ||
| <nav className="panel"> | ||
| <p className="panel-heading">Filters</p> | ||
|
|
||
| <p className="panel-tabs" data-cy="SexFilter"> | ||
| <a className="is-active" href="#/people"> | ||
| <a | ||
| className={sex === 'all' ? 'is-active' : ''} | ||
| onClick={() => setSex('all')} | ||
| > | ||
| All | ||
| </a> | ||
| <a className="" href="#/people?sex=m"> | ||
| <a | ||
| className={sex === 'm' ? 'is-active' : ''} | ||
| onClick={() => setSex('m')} | ||
| > | ||
| Male | ||
| </a> | ||
| <a className="" href="#/people?sex=f"> | ||
| <a | ||
| className={sex === 'f' ? 'is-active' : ''} | ||
| onClick={() => setSex('f')} | ||
| > | ||
| Female | ||
| </a> | ||
| </p> | ||
|
|
@@ -22,6 +95,8 @@ export const PeopleFilters = () => { | |
| type="search" | ||
| className="input" | ||
| placeholder="Search" | ||
| value={searchParams.get('query') || ''} | ||
| onChange={handleQueryChange} | ||
| /> | ||
|
|
||
| <span className="icon is-left"> | ||
|
|
@@ -35,40 +110,40 @@ export const PeopleFilters = () => { | |
| <div className="level-left"> | ||
| <a | ||
| data-cy="century" | ||
| className="button mr-1" | ||
| href="#/people?centuries=16" | ||
| className={`button mr-1 ${isSelected(16) ? 'is-info' : ''}`} | ||
| onClick={() => toggleCentury(16)} | ||
| > | ||
| 16 | ||
| </a> | ||
|
|
||
| <a | ||
| data-cy="century" | ||
| className="button mr-1 is-info" | ||
| href="#/people?centuries=17" | ||
| className={`button mr-1 ${isSelected(17) ? 'is-info' : ''}`} | ||
| onClick={() => toggleCentury(17)} | ||
| > | ||
| 17 | ||
| </a> | ||
|
|
||
| <a | ||
| data-cy="century" | ||
| className="button mr-1 is-info" | ||
| href="#/people?centuries=18" | ||
| className={`button mr-1 ${isSelected(18) ? 'is-info' : ''}`} | ||
| onClick={() => toggleCentury(18)} | ||
| > | ||
| 18 | ||
| </a> | ||
|
|
||
| <a | ||
| data-cy="century" | ||
| className="button mr-1 is-info" | ||
| href="#/people?centuries=19" | ||
| className={`button mr-1 ${isSelected(19) ? 'is-info' : ''}`} | ||
| onClick={() => toggleCentury(19)} | ||
| > | ||
| 19 | ||
| </a> | ||
|
|
||
| <a | ||
| data-cy="century" | ||
| className="button mr-1" | ||
| href="#/people?centuries=20" | ||
| className={`button mr-1 ${isSelected(20) ? 'is-info' : ''}`} | ||
| onClick={() => toggleCentury(20)} | ||
| > | ||
| 20 | ||
| </a> | ||
|
|
@@ -77,8 +152,8 @@ export const PeopleFilters = () => { | |
| <div className="level-right ml-4"> | ||
| <a | ||
| data-cy="centuryALL" | ||
| className="button is-success is-outlined" | ||
| href="#/people" | ||
| className={`button is-success ${selectedCenturies.length !== 0 ? 'is-outlined' : ''}`} | ||
| onClick={resetCenturies} | ||
| > | ||
| All | ||
| </a> | ||
|
|
@@ -87,7 +162,10 @@ export const PeopleFilters = () => { | |
| </div> | ||
|
|
||
| <div className="panel-block"> | ||
| <a className="button is-link is-outlined is-fullwidth" href="#/people"> | ||
| <a | ||
| className="button is-link is-outlined is-fullwidth" | ||
| onClick={resetAllFilters} | ||
| > | ||
| Reset all filters | ||
| </a> | ||
| </div> | ||
|
|
||
This file was deleted.
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.
This link navigates to
/people/${person.slug}without preserving search params. According to checklist item #3, search params must be preserved when navigating within the People page (selecting a person). Use SearchLink component with preserved params instead.