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

Venue availability information page #498

Merged
merged 24 commits into from Dec 11, 2017

Conversation

Projects
None yet
5 participants
@taneliang
Copy link
Member

taneliang commented Dec 9, 2017

New Venues page. The UI is... rather focused on functionality at the expense of form.

localhost_8080_venues_t

Features

  • Adds route to /venues/:venue?
  • Displays a list of venues at /venues
  • Venues searchable by venue name. Search is case-insensitive
  • Venues in list expand to show their classes in a timetable. Only one venue can be expanded at any time
  • Handles back/forward navigation
  • Tests included

Venues URL

Future Work

The scope of this PR does not include searching venues by their availability - this will be implemented in a future PR.

@mods-bot

This comment has been minimized.

Copy link

mods-bot commented Dec 9, 2017

Deploy preview ready!

Built with commit 38f1106

https://deploy-preview-498--nusmods.netlify.com

@ZhangYiJiang

This comment has been minimized.

Copy link
Contributor

ZhangYiJiang commented Dec 10, 2017

List of bugs - don't worry about fixing all of them, this is just to keep track of them so they can be fixed at some point

  • Scroll position needs to be updated when opening/closing venues, otherwise the difference in height can cause the newly opened timetable to shift off screen
  • Timetable coloring is not done correctly - the colors need to be assigned per module, not per lesson type
  • Timetable arrangement seems incorrect for some venues - eg. https://deploy-preview-498--nusmods.netlify.com/venues/dRm3
  • Search results need to be sorted
  • Search query needs tokenization
@ZhangYiJiang
Copy link
Contributor

ZhangYiJiang left a comment

The code looks fine for the most part, but I have concerns about how URL and search state syncs up. The problem is that as you search, the URL is not updated until you actually select a venue.

This may be okay when searching by name, but when searching by time you need that information to be encoded in the URL if you want to share it. I would suggest using query string to do so - this would also remove the need for the componentWillReceiveProps hack in VenuesContainer

@@ -0,0 +1,22 @@
@import "~styles/bootstrap/variable-overrides.scss";

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 10, 2017

Contributor

Should import from ~styles/utils/modules-entry.scss

margin-top: 0.25rem;
}

&:not(:last-child) {

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 10, 2017

Contributor

Simpler to give every row a border-top, then set border-top: 0 on :first-child


const lowercaseSearchStr = searchTerm.toLowerCase();
return pick(venues, Object.keys(venues).filter(name =>
name.toLowerCase().indexOf(lowercaseSearchStr) !== -1));

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 10, 2017

Contributor

Can use String#includes

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 10, 2017

Coverage Status

Coverage increased (+4.3%) to 39.905% when pulling 7e69ad6 on taneliang:venue-info-page into 428c6e2 on nusmodifications:master.

@taneliang

This comment has been minimized.

Copy link
Member Author

taneliang commented Dec 10, 2017

@ZhangYiJiang To summarize, should the behavior be:

  • As user searches, URL will be updated to /venues?q=<searchTerm>, with &day=<searchDay>&startTime=<startTime>&endTime=<endTime> following in the future.
  • When a venue is selected, URL will be updated to /venues/<venue>?q=<searchTerm>&...
  • The individual query params will only be present if set by the user
@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 10, 2017

Coverage Status

Coverage increased (+4.3%) to 39.905% when pulling 56dce71 on taneliang:venue-info-page into 428c6e2 on nusmodifications:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 10, 2017

Coverage Status

Coverage increased (+4.3%) to 39.905% when pulling 2448140 on taneliang:venue-info-page into 428c6e2 on nusmodifications:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 10, 2017

Coverage Status

Coverage increased (+5.0%) to 40.621% when pulling 7f3dc5a on taneliang:venue-info-page into 428c6e2 on nusmodifications:master.

@taneliang

This comment has been minimized.

Copy link
Member Author

taneliang commented Dec 10, 2017

I don't think the timetable rendering issue is caused by this PR. Maybe that should be fixed in a separate PR to limit the scope of this PR?

For the scroll position updating, does anyone know of a non-hacky way to scroll the element below the search box? I have a rather hacky solution for that by just moving the element down by 40px, but this is inelegant and doesn't work super well as the search box is 30px tall on mobile and 40px on tablets and up.

@ZhangYiJiang

This comment has been minimized.

Copy link
Contributor

ZhangYiJiang commented Dec 10, 2017

To summarize, should the behavior be: [...]

Sounds correct, but I'm not sure what you mean by

The individual query params will only be present if set by the user


I don't think the timetable rendering issue is caused by this PR.

I'll throw it onto the backlog then.


For the scroll position updating, does anyone know of a non-hacky way to scroll the element below the search box?

The best way would actually be to read the actual height of the nav header DOM element. Barring that, you can use the media query util functions to select the correct height. Or you could just use a large enough number that it would always appear on screen (on larger screens the top will not be aligned properly, but it's a very minor detail). There are also CSS-based solutions, but they're not very clean (add a padding equal to the nav height, then use negative margin to offset that).

@taneliang

This comment has been minimized.

Copy link
Member Author

taneliang commented Dec 10, 2017

Sounds correct, but I'm not sure what you mean by

The individual query params will only be present if set by the user

Sorry the phrasing there was unclear. What I meant was like q=<searchTerm> will only be present if the user typed something into the search box. So if the user only selected a time range, the URL will be /venues?startTime=<startTime>&endTime=<endTime>.

The best way would actually be to read the actual height of the nav header DOM element.

Okay I'll do it this way. Not sure why I didn't consider this method. Thanks!

Also one more thing - the color coding of the modules may be confusing as many venues are occupied by more than 8 modules, but there are only 8 colors. v2 avoided that by simply not coloring anything. I think it's clearer with colors than without, but some may find the colors misleading. Thoughts?

image

@ZhangYiJiang

This comment has been minimized.

Copy link
Contributor

ZhangYiJiang commented Dec 10, 2017

What I meant was like q= will only be present if the user typed something into the search box. So if the user only selected a time range, the URL will be /venues?startTime=&endTime=.

Yup, that's exactly right.

Also one more thing - the color coding of the modules may be confusing as many venues are occupied by more than 8 modules, but there are only 8 colors. v2 avoided that by simply not coloring anything. I think it's clearer with colors than without, but the colors may also be misleading. Thoughts?

Well, in the example you have above the problem is caused by double coded GE modules (although I think the problem will remain even after deduplicating them). We'll need some empirical evidence (how many venues suffer from this problem, and confused do users get), but my intuition is that coloring is better, so let's just go with that for now.

@taneliang

This comment has been minimized.

Copy link
Member Author

taneliang commented Dec 10, 2017

coloring is better, so let's just go with that for now

Agreed. Anyway I've counted, and 259 out of 621 venues have more than 8 unique modules in AY17/18 sem 1, and 253/561 venues also suffer from this in sem 2.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 10, 2017

Coverage Status

Coverage increased (+5.02%) to 40.669% when pulling 155f61e on taneliang:venue-info-page into c3c5c1a on nusmodifications:master.

@@ -0,0 +1,71 @@
// @flow

This comment has been minimized.

@yangshun

yangshun Dec 10, 2017

Member

Thanks for adding tests 😄

}

const availability: DayAvailability[] = this.props.availability;
// const lessons = flatMap(availability, a => a.Classes) // Not using flatMap as it results in a Flow error

This comment has been minimized.

@yangshun

yangshun Dec 10, 2017

Member

Remove this if not using.

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 10, 2017

Contributor

Actually, if this is a libdef issue, it may be better to file issue/PR against https://github.com/flowtype/flow-typed, and just use a $FlowFIxMe to ignore the error while the issue/PR is getting processed.

This comment has been minimized.

@taneliang

taneliang Dec 11, 2017

Author Member

I'm not familiar enough with Flow to know if it's an issue with libdef though. I'll just change it to $FlowFixMe first and figure this out later

}}
>{name}</a>
</h4>
{lessons ? (

This comment has been minimized.

@yangshun

yangshun Dec 10, 2017

Member

lessons && (<Component />) is shorter.

}

componentDidMount() {
axios.get(nusmods.venuesUrl(this.props.activeSemester))

This comment has been minimized.

@yangshun

yangshun Dec 10, 2017

Member

This is fine for now but I think we should consider putting the venues data into the store so that it can be potentially reused in other places.

}

a {
width: 100%;

This comment has been minimized.

@yangshun

yangshun Dec 10, 2017

Member

I don't think this does anything as <a> is display: inline and setting a width does nothing to an inline element. Might want to use display: block instead or try out the comment where I suggested swapping the <h4> and the <a>.

const venues = this.filteredVenues();

return (
<div className="modules-page-container page-container">

This comment has been minimized.

@yangshun

yangshun Dec 10, 2017

Member

Is it really meant to be .modules-page-container or is it a copy paste error? 😂

This comment has been minimized.

@taneliang

taneliang Dec 11, 2017

Author Member

Derp

expect(Object.keys(component.filteredVenues())).toEqual(['LT17', 'LT1']);

// Venue which does not exist
component.setState({ searchTerm: 'covfefe' });

This comment has been minimized.

@yangshun

yangshun Dec 10, 2017

Member

😂

expect(component.props.history.location.pathname).toEqual('/venues/LT17');
});

describe('URL handling', () => {

This comment has been minimized.

@yangshun

yangshun Dec 10, 2017

Member

Do we wanna test a case where a venue contains a slash?

@yangshun
Copy link
Member

yangshun left a comment

This is really good work. Well done @taneliang ! Other things to consider:

For the scroll position updating, does anyone know of a non-hacky way to scroll the element below the search box? I have a rather hacky solution for that by just moving the element down by 40px, but this is inelegant and doesn't work super well as the search box is 30px tall on mobile and 40px on tablets and up.

I wouldn't be bothered by this. Set it to whichever values works ok for all device widths. The difference hardly matters and people might not even be expecting nicely flushed offset in the first place.

Also one more thing - the color coding of the modules may be confusing as many venues are occupied by more than 8 modules, but there are only 8 colors. v2 avoided that by simply not coloring anything. I think it's clearer with colors than without, but some may find the colors misleading. Thoughts?

Repeating colors is fine. Alternatively, use gray color background like in v2 and only change the color of all cells of the same module on hover (won't work on mobile though).


Lastly, consider automatically setting the venue timetable to vertical orientation when viewed on mobile. Great work overall. Will leave to @ZhangYiJiang to do the final approval 👍

throttle={0}
useInstantSearch
initialSearchTerm={this.state.searchTerm}
placeholder="Venues"

This comment has been minimized.

@yangshun

yangshun Dec 10, 2017

Member

I think a more descriptive placeholder will be better. How about "Search for venues. E.g. LT27"?

a.toLowerCase().localeCompare(b.toLowerCase(), 'en', { numeric: true }));

return (
<ul className={styles.venueList}>

This comment has been minimized.

@yangshun

yangshun Dec 10, 2017

Member

For clarity, it would be good to show a placeholder "No venues found matching " when there are 0 results.

return (
<li className={styles.venueDetailRow} ref={rootElementRef}>
<h4>
<a

This comment has been minimized.

@yangshun

yangshun Dec 10, 2017

Member

Consider wrapping <h4> within <a> so that it automatically stretches across the whole row without the need for modifying <a>'s styles.

This comment has been minimized.

@taneliang

taneliang Dec 11, 2017

Author Member

That's exactly what I was trying to do! Thanks

@yangshun yangshun added the v3 label Dec 11, 2017

@taneliang

This comment has been minimized.

Copy link
Member Author

taneliang commented Dec 11, 2017

I've implemented the live query string update. This is the behavior:

  • Loading from URL:
    • /venues/<string> -> /venues/<string>?q=<string>. i.e. navigating to a venue selects venue and filters list by venue name
    • /venues/<string>?q=<query> will select venue and filter list by query
  • Clicking on venue:
    • /venues?q=<query> OR /venues/<string>?q=<query> ---click--> /venues/<venue>?q=<query>

However, if the user searches for lt, then clicks on LT17, and shares the resulting URL /venues/LT17?q=lt, the recipient will see the same venue in a list. This seems a bit strange to me. I'm not sure when anyone will want to share the filtered list when they have just clicked on a venue.

Perhaps we should remove the query part from the URL when a venue is clicked? i.e. /venues?q=<query> ---click--> /venues/LT17 ---query changed--> /venues/LT17?q=<newquery>.

@ZhangYiJiang
Copy link
Contributor

ZhangYiJiang left a comment

Good job! Most of these are very minor, so I think this PR is almost ready to be merged


// Case-insensitive, natural sort of venue names
const sortedVenueNames = Object.keys(venues).sort((a, b) =>
a.toLowerCase().localeCompare(b.toLowerCase(), 'en', { numeric: true }));

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 11, 2017

Contributor

Nice - although you can use sensitivity: base to avoid having to convert everything to lower case. The docs for this also note that you should create an Intl.Collator object if you're sorting strings, so that might be worth checking out

render() {
const { name, onClick } = this.props;
const lessons = this.arrangedLessons();
const venueHref = `/venues/${encodeURIComponent(name)}`;

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 11, 2017

Contributor

Consider extracting this out to routes/paths.js


const availability: DayAvailability[] = this.props.availability;
// $FlowFixMe
const lessons = flatMap(availability, a => a.Classes)

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 11, 2017

Contributor

Ah, I see what Flow is complaining about - basically, there's two different type definitions for flatMap. Flow can't infer which one to use. To resolve this, just add enough annotations to help the inference engine out -

const lessons = flatMap(availability, (day): VenueLesson[] => day.Classes)
const { name, onClick } = this.props;
const lessons = this.arrangedLessons();
const venueHref = `/venues/${encodeURIComponent(name)}`;
const rootElementRef: Function = this.props.rootElementRef || (() => {});

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 11, 2017

Contributor

Why not also use defaultProps for this?

const lowercaseExpandedVenue = expandedVenue.toLowerCase();

// Case-insensitive, natural sort of venue names
const sortedVenueNames = Object.keys(venues).sort((a, b) =>

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 11, 2017

Contributor

Perhaps _.sortBy is a better option here - I'm surprised Flow isn't complaining about your use of venues[name] below, since that should produce an optional, which does not match the prop type defined for the component.

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 11, 2017

Contributor

Ah, sortBy only provides value to the iterating function. I guess you could use _.entries, but this is fine too I guess.


.venuesPageContainer {
margin-left: 0;
animation-name: $page-entering-animation;

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 11, 2017

Contributor

Need to use :global, otherwise the animation name will be incorrectly mangled

.toContain(getNewColor(currentColors, true));
}

for (let i = 0; i < 100; i++) {

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 11, 2017

Contributor

/nit Since you already have _.range imported, personally I think range(100).forEach is a bit more readable (or maybe I'm just too used to Python 😆)

This comment has been minimized.

@taneliang

taneliang Dec 11, 2017

Author Member

Definitely. I'm already doing that on line 58. Probably wrote this line before I imported range 😆

</div>
);
return <Warning message="No modules found" />;
// return (

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 11, 2017

Contributor

Need to remove this before merging


test('it displays warning triangle and message', () => {
const wrapper = shallow(<Warning message="message" />);
expect(wrapper.find('AlertTriangle')).toHaveLength(1);

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 11, 2017

Contributor

This test seems overspecified. I don't think it's useful to test presentational elements like the warning triangle icon.


.venueDetailRow {
padding-top: 1rem;
border-top: 1px $gray-lighter solid;

This comment has been minimized.

@ZhangYiJiang

ZhangYiJiang Dec 11, 2017

Contributor

Should be var(--gray-lighter) now that the dark mode PR has been merged in

@ZhangYiJiang

This comment has been minimized.

Copy link
Contributor

ZhangYiJiang commented Dec 11, 2017

Perhaps we should remove the query part from the URL when a venue is clicked? i.e. /venues?q= ---click--> /venues/LT17 ---query changed--> /venues/LT17?q=.

My intuition says this will cause problems, because you're desynchronizing URL state from UI state, which will cause problems like the last time. For the time being just leave it - we can revise if it causes problems later.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 11, 2017

Coverage Status

Coverage increased (+5.06%) to 44.895% when pulling b9a5fb3 on taneliang:venue-info-page into bffb5f0 on nusmodifications:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 11, 2017

Coverage Status

Coverage increased (+5.09%) to 44.925% when pulling 38f1106 on taneliang:venue-info-page into bffb5f0 on nusmodifications:master.

@ZhangYiJiang ZhangYiJiang merged commit 08eb556 into nusmodifications:master Dec 11, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+5.09%) to 44.925%
Details
deploy/netlify Deploy preview ready!
Details

@taneliang taneliang deleted the taneliang:venue-info-page branch Dec 12, 2017

@taneliang taneliang referenced this pull request Dec 12, 2017

Closed

Port venue information page to v3 #315

3 of 3 tasks complete

@ZhangYiJiang ZhangYiJiang referenced this pull request Dec 16, 2017

Closed

Blockers for NUSMods R release #464

20 of 24 tasks complete

@yangshun yangshun referenced this pull request Dec 24, 2017

Closed

Port venue availability page to v3 #314

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment