-
Notifications
You must be signed in to change notification settings - Fork 3
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
add query builder #294
base: as66/search-filters
Are you sure you want to change the base?
add query builder #294
Conversation
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.
Make code more readable and break things into smaller components/functions. Rerequest me when done.
The keyword doesn't seem to work. I will link the issue manually |
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.
Some of the feedback was given synchronously during the dev session.
TL;DR
- state management without duplication
- automatic class year calculation (no hardcoded class years)
const [year27Selected, setYear27Selected] = useState(false); | ||
const [year26Selected, setYear26Selected] = useState(false); | ||
const [year25Selected, setYear25Selected] = useState(false); | ||
const [year24Selected, setYear24Selected] = useState(false); |
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 years can be autogenerated. These states should be interpreted as first-years/sophomores/juniors/seniors, and then added by the base academic year.
The academic year can be calculated with these codes here. We may want to turn it into a util function for reuse.
wso-react/src/constants/constants.js
Lines 25 to 31 in d82b8ce
let now = new Date(); | |
// year represents the higher end of academic year | |
// e.g. 2024 represents the AY 2023-2024 | |
let year = now.getFullYear(); | |
if (now.getMonth() >= 5) { | |
year++; | |
} |
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.
Also, the state here could simply be a number, or an enum of four options (preferred) if you want.
const [search, updateSearch] = useState(""); | ||
const [query, updateQuery] = useState(""); |
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 is an interesting design. However, I think query
and search
can be merged as one, where updates directly shows up in the query box.
That being said, we can offer user a separate textbox in the advanced search options.
Think of the "all these words: " box in Google's Advanced Search page.
let firstYear = 1; | ||
if (!year27Selected) { | ||
firstYear = 2; | ||
if (!year26Selected) { | ||
firstYear = 3; | ||
if (!year25Selected) { | ||
firstYear = 4; | ||
} | ||
} | ||
} | ||
if (year27Selected || year26Selected || year25Selected || year24Selected) | ||
str += prior ? " AND (" : "("; | ||
if (year27Selected) str += 'year: "27"'; | ||
if (year26Selected) str += firstYear === 2 ? 'year: "26"' : ' OR year: "26"'; | ||
if (year25Selected) str += firstYear === 3 ? 'year: "25"' : ' OR year: "25"'; | ||
if (year24Selected) str += firstYear === 4 ? 'year: "24"' : ' OR year: "24"'; | ||
if (year27Selected || year26Selected || year25Selected || year24Selected) { | ||
str += ")"; | ||
prior = true; | ||
} |
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.
As in previous comment, turn the state into a mathematical term to be added to the base academic year.
const [search, updateSearch] = useState(""); | ||
const [query, updateQuery] = useState(""); | ||
const [filtersClickedOff, setFiltersClickedOff] = useState(false); | ||
|
||
const [advancedFiltersSelected, setAdvancedFiltersSelected] = useState(false); | ||
const [name, updateName] = useState(""); | ||
const [unix, updateUnix] = useState(""); | ||
const [country, updateCountry] = useState(""); | ||
const [state, updateState] = useState(""); | ||
const [city, updateCity] = useState(""); | ||
const [year27Selected, setYear27Selected] = useState(false); | ||
const [year26Selected, setYear26Selected] = useState(false); | ||
const [year25Selected, setYear25Selected] = useState(false); | ||
const [year24Selected, setYear24Selected] = useState(false); | ||
const [professorSelected, setProfessorSelected] = useState(false); | ||
const [staffSelected, setStaffSelected] = useState(false); | ||
const [studentSelected, setStudentSelected] = useState(false); | ||
const [building, updateBuilding] = useState(""); |
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.
Having these states stored duplicated at two places doesn't seem ideal.
We can maybe use a global state (like a redux slice that isn't persisted), or manually pass these down as props (would be a lot of painful work IMO)
@AndrewMuh thoughts?
Resolves #172