{"action":"created","issue":{"url":"https://api.github.com/repos/nusmodifications/nusmods/issues/441","repository_url":"https://api.github.com/repos/nusmodifications/nusmods","labels_url":"https://api.github.com/repos/nusmodifications/nusmods/issues/441/labels{/name}","comments_url":"https://api.github.com/repos/nusmodifications/nusmods/issues/441/comments","events_url":"https://api.github.com/repos/nusmodifications/nusmods/issues/441/events","html_url":"https://github.com/nusmodifications/nusmods/pull/441","id":255629819,"number":441,"title":"Modules page filters part four: Infinite scroll, sticky sidebar ","user":{"login":"ZhangYiJiang","id":445650,"avatar_url":"https://avatars0.githubusercontent.com/u/445650?v=4","gravatar_id":"","url":"https://api.github.com/users/ZhangYiJiang","html_url":"https://github.com/ZhangYiJiang","followers_url":"https://api.github.com/users/ZhangYiJiang/followers","following_url":"https://api.github.com/users/ZhangYiJiang/following{/other_user}","gists_url":"https://api.github.com/users/ZhangYiJiang/gists{/gist_id}","starred_url":"https://api.github.com/users/ZhangYiJiang/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ZhangYiJiang/subscriptions","organizations_url":"https://api.github.com/users/ZhangYiJiang/orgs","repos_url":"https://api.github.com/users/ZhangYiJiang/repos","events_url":"https://api.github.com/users/ZhangYiJiang/events{/privacy}","received_events_url":"https://api.github.com/users/ZhangYiJiang/received_events","type":"User","site_admin":false},"labels":[{"id":441933196,"url":"https://api.github.com/repos/nusmodifications/nusmods/labels/v3","name":"v3","color":"0e8a16","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2017-09-06T14:43:14Z","updated_at":"2017-09-08T16:00:55Z","closed_at":null,"author_association":"COLLABORATOR","pull_request":{"url":"https://api.github.com/repos/nusmodifications/nusmods/pulls/441","html_url":"https://github.com/nusmodifications/nusmods/pull/441","diff_url":"https://github.com/nusmodifications/nusmods/pull/441.diff","patch_url":"https://github.com/nusmodifications/nusmods/pull/441.patch"},"body":"This is part of the project to bring the module search page at `/modules` to feature parity for v3. See https://github.com/nusmodifications/nusmods/issues/426 for the other PRs in this series. \r\n\r\n### Infinite scroll\r\n\r\nThis is the big feature for this PR. Infinite scroll vs. a more traditional page based view is controversial, and hard to get right. The current implementation is experimental - if it is not up to scratch, it should not be too difficult to pull out the infinite scroll and replace it with a more traditional pagination navigation. That said, I believe the current implementation is **as good as pagination** (this is my litmus test for whether infinite scroll should be kept) in almost all aspects, and works better in a lot others, especially on mobile. \r\n\r\n#### Approach \r\n\r\nThe `ModuleFinderList` component handles the splitting of the modules array into pages (chunks of 10 modules). [React Waypoint](https://github.com/brigade/react-waypoint) wraps every page to track which page the user is looking at. \r\n\r\nWhen the user reaches the last visible page, the number of visible pages is incremented, causing the next set of modules to be rendered. Page state is kept in `ModuleFinderContainer`, which keep tracks of the user's current page, the first rendered page and the number of visible pages. If the user changes filters, the current page resets to zero and we scroll back to top.\r\n\r\nReact Virtualized (and other off the shelf lazily rendered infinite scroll solutions) cannot be used because we have variable list item heights. \r\n\r\n#### Navigation \r\n\r\n![screenshot from 2017-09-06 20-45-42](https://user-images.githubusercontent.com/445650/30117831-f4c411e2-9353-11e7-8ddf-aabcf5aba17e.png)\r\n\r\nDividers are inserted between pages to orientate the user by showing them how many items they've looked at. To preserve the user's current page, as the user scrolls, the URL hash is updated. When the user returns, we only show modules from the current page onward. \r\n\r\n![localhost-8080-modules-level 2](https://user-images.githubusercontent.com/445650/30113261-aceea38a-9346-11e7-9f9a-9d085eaaf510.png)\r\n\r\nThe user has to manually trigger the loading of previous pages. This removes the need for us to have to worry about having to manually manage scroll position, which would be difficult and fragile. \r\n\r\n#### Performance \r\n\r\nInitial performance testing using a local production build on mobile and desktop Chrome shows no stuttering and lag during scrolling. \r\n\r\n#### Remaining issue \r\n\r\nThe one unavoidable issue is the scrollbar position, which jumps as the new page is rendered. I don't personally think it is a dealbreaker, but we need to do more user testing to figure this out, as well as if this implementation is usable in general. \r\n\r\n### Style updates \r\n\r\nThe other part of this PR is to get the styling of various UI elements closer to a release ready stage. \r\n\r\n#### Sticky filter sidebar\r\n\r\nOn desktop the design uses Amazon's product search sidebar as reference, so the changes are mostly minor tweaks. On mobile the [Google IO conference session page](https://events.google.com/io/schedule/?section=may-17) (remember to resize down to mobile size - the design changes significantly between mobile and desktop) filter controls is used as reference. \r\n\r\n![localhost-8080-modules-lecture tuesday-morning mc 4 iphone 6 plus](https://user-images.githubusercontent.com/445650/30113260-acee1384-9346-11e7-9866-ee821cf333ca.png)\r\n\r\nNote that this is styling only - the popup behavior currently uses `:hover`, which is a placeholder until a proper component can be written for this. \r\n\r\n#### Semester info selector \r\n\r\nThe semester info selector need to fulfill two use cases \r\n\r\n- Tell the user which semester the module is available in \r\n- Allow the user to check class info in another semester \r\n\r\nThis means it needs to function as a set of three-state tabs, which is somewhat complex as far as UI elements go. This is what it looks like now \r\n\r\n![screenshot from 2017-09-06 20-37-19](https://user-images.githubusercontent.com/445650/30117894-2b791e44-9354-11e7-9dda-7225890d4476.png)\r\n\r\nthe tooltip should at some point be replaced with a proper tooltip component, like say Popper. \r\n\r\n### Other changes \r\n\r\n- Stylelint has been reduced to warning if the env is not production. This reduces development friction, since most of its complaints are stylistic and can be fixed before commit\r\n- Fix `LinkModuleCodes` key warning caused by the same module code appearing more than once\r\n"},"comment":{"url":"https://api.github.com/repos/nusmodifications/nusmods/issues/comments/328144365","html_url":"https://github.com/nusmodifications/nusmods/pull/441#issuecomment-328144365","issue_url":"https://api.github.com/repos/nusmodifications/nusmods/issues/441","id":328144365,"user":{"login":"coveralls","id":2354108,"avatar_url":"https://avatars1.githubusercontent.com/u/2354108?v=4","gravatar_id":"","url":"https://api.github.com/users/coveralls","html_url":"https://github.com/coveralls","followers_url":"https://api.github.com/users/coveralls/followers","following_url":"https://api.github.com/users/coveralls/following{/other_user}","gists_url":"https://api.github.com/users/coveralls/gists{/gist_id}","starred_url":"https://api.github.com/users/coveralls/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/coveralls/subscriptions","organizations_url":"https://api.github.com/users/coveralls/orgs","repos_url":"https://api.github.com/users/coveralls/repos","events_url":"https://api.github.com/users/coveralls/events{/privacy}","received_events_url":"https://api.github.com/users/coveralls/received_events","type":"User","site_admin":false},"created_at":"2017-09-08T16:00:55Z","updated_at":"2017-09-08T16:00:55Z","author_association":"NONE","body":"\n[![Coverage Status](https://coveralls.io/builds/13187988/badge)](https://coveralls.io/builds/13187988)\n\nCoverage increased (+1.2%) to 29.167% when pulling **19d682dac5c619e722097ed18aef386748366ca5 on modules-page-filters-part-four** into **3a82c37c0c824081aeaf3e2da0c280c47b947f6c on master**.\n"}}
{"actor":{"display_login":"coveralls"}}