-
Notifications
You must be signed in to change notification settings - Fork 16
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
[WIP] Add filters to course search #2378
Conversation
I can also move the rest of the course search redux stuff into the course-search part in this PR... @hawkrives I know you mentioned wanting that to happen. |
Oh, I do like showing a summary of the selected filters in the filter space. That's a great idea! I'm sure we can come up with a way to represent them compactly – |
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.
Wow! Nice work.
source/lib/course-search/index.js
Outdated
@@ -4,3 +4,4 @@ export {loadCachedCourses} from './load-cached-courses' | |||
export {updateStoredCourses, areAnyTermsCached} from './update-course-storage' | |||
export {CourseType, TermType} from './types' | |||
export {parseTerm} from './parse-term' | |||
export {loadGEs} from './load-ges' |
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.
Where is this called?
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.
Looks to have been introduced in d2fdcdb#diff-df4b353eb39c9a68e6b0e3127a055bb4R69
and removed in da79336#diff-df4b353eb39c9a68e6b0e3127a055bb4L69
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.
Yeah that was left over from my first attempt when I wasn't reusing the filter component..I kept it so that I could ask this question: For access to the acceptable list of GEs, departments, etc. should we load that data from the server and store it just like the course data, or should we try to somehow extract them from the course data itself? @hawkrives
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.
I'd recommend using https://github.com/StoDevX/course-data/blob/gh-pages/data-lists/valid_gereqs.json for a list of GE requirements.
It's generated automatically from the course data every night.
Of, you can do something to determine the gereqs from the downloaded course data; that's what I do in Gobbldygook, too.
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.
That's what it uses currently.
@@ -6,7 +6,8 @@ export type ToggleSpecType = { | |||
} | |||
|
|||
export type ListItemSpecType = {| | |||
title: string, | |||
title: string | number, | |||
label?: string, |
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.
I'm not entirely clear on the difference between these two – can you summarize for me, please?
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.
Looks like it comes down to:
title: term.term,
label: parseTerm(term.term.toString()),
term
would be a string or a numberlabel
is data massaged out ofparseTerm
which lives in lib/course-search/parse-term.js where it returns the human readable term name (Abroad current / next year, Fall current / next year, etc)
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.
Yep! I changed the spec type so that the applyFilters() function would work when the value of the key item specified isn't a string. The label allows the filter options to be displayed as "Fall 2016/17" instead of 20171, for example.
@hawkrives re:summary of selected filters |
@hawkrives Ok so I got a filter up and running for "Open Courses," and realized that there are a whole bunch of courses listed as "Open" from past semesters...is this something that you want to fix for the course-data on the server itself or should that logic go in AAO? |
@hannesmcman Those courses are technically still "open", as far as SIS / the registrar are concerned. I don't know that we want to change that? I'd still question how useful an open/closed filter would be. I never ran into a situation where it was a useful distinction to me as a student, AFAIR? |
I say we shouldn't return past semester courses that are listed as open on the app's end. |
I say we shouldn't care about "open/closed" at all :p |
...and I see plenty of use for an open/closed filter for those trying to find open sections. |
I don't even know when a section gets "closed". I don't remember seeing any that were closed in anything that I wanted to take (cough CS /cough), even when they were full. |
I was kicked out of a section that had closed during registration and had to find another section that was open |
New dependencies added: p-props. p-propsAuthor: Sindre Sorhus Description: Like `Promise.all()` but for `Map` and `Object` Homepage: https://github.com/sindresorhus/p-props#readme
|
package.json
Outdated
@@ -92,6 +92,7 @@ | |||
"@hawkrives/react-native-alphabetlistview": "1.0.0", | |||
"@hawkrives/react-native-alternate-icons": "0.4.7", | |||
"@hawkrives/react-native-sortable-list": "1.0.1", | |||
"bluebird": "3.5.1", |
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.
Can you use https://www.npmjs.com/package/p-props instead, please? It's a more focused dependency.
case '9': | ||
return {year: `${currentYearAbbrev}/${nextYear}`, semester: 'Non-Sto'} | ||
default: | ||
return {year: `${currentYearAbbrev}/${nextYear}`, semester: 'Unk'} |
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.
N/A might work well here. @StoDevX/all-about-olaf anyone have an opinion?
* master: (43 commits) chore(package): update lockfile fix(package): update react-native-network-info to version 3.1.0 update babel-jest to 22.4.1 update react-native-device-info to 0.17.3 run prettier update prettier to 1.11.1 chore(package): update lockfile chore(package): update js-yaml to version 3.11.0 chore(package): update lockfile chore(package): update ajv to version 6.2.1 chore(package): update lockfile fix(package): update moment to version 2.21.0 chore(package): update lockfile chore(package): update eslint to version 4.18.2 fix indentation make hawken happy and bring back cell Add url parsing back to helper for android to use Remove duplicate style Bundle Update on 2018-03-02 Replace org detail meetings, description, and site texts with TextInput ...
This change will decrease the build size from 52.32 MB to 18.65 MB, a decrease of 33.67 MB (181%)
|
I did a fair amount of reading on Flow's "disjoint union types" tonight, trying to figure out what was going on, but I think it's actually this bug, instead: Refine array types using So, what I did – and I only did this because this is an exceptional case; we want to avoid export function filterListSpecs(specs: Array<FilterType>): Array<ListType> {
const retval = specs.filter(f => f.type === 'list')
return ((retval: any): Array<ListType>)
} I filter the array, with hopefully bug-free code, then I apply a typecast to the result to tell Flow exactly what we're returning. |
iOS ReportAnalysis of slow build times (>20s)
Generated by 🚫 dangerJS |
Resolves #2342.
Desired filters: