Adds sessions layout public event #68

dilpreetsio · 2017-05-22T04:58:00Z

I have read the Contribution & Best practices Guide.
My branch is up-to-date with the Upstream development branch.
The acceptance, integration, unit tests and linter pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Short description of what this resolves:

Adds session layout in public events page
Added components

Session-filter
Session-list
Session-item

####Screenshot

Fixes #63
@niranjan94 Please review.

codecov · 2017-05-22T05:04:28Z

Codecov Report

Merging #68 into development will increase coverage by 0.17%.
The diff coverage is 77.77%.

@@               Coverage Diff               @@
##           development      #68      +/-   ##
===============================================
+ Coverage        58.83%   59.01%   +0.17%     
===============================================
  Files              129      133       +4     
  Lines              945      954       +9     
===============================================
+ Hits               556      563       +7     
- Misses             389      391       +2

Impacted Files	Coverage Δ
app/components/public/session-list.js	`100% <100%> (ø)`
app/components/public/session-item.js	`100% <100%> (ø)`
app/components/public/session-filter.js	`50% <50%> (ø)`
app/controllers/public/sessions.js	`80% <80%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb8eeb0...9c48c91. Read the comment docs.

lunaticmonk · 2017-05-22T05:27:03Z

@geekyd Looks good, but I think it would look better if we increase some space between the "Talk" and the image.

niranjan94

A few functional changes. UI review I'll do it once this is done.

niranjan94 · 2017-05-22T05:36:51Z

app/controllers/public/sessions.js

+const { Controller, computed } = Ember;
+
+export default Controller.extend({
+  sessions: [{ shortAbstract: 'An introduction to the event', id: '1', title: 'Welcome to FOSSASIA', startAt: '10:35 AM / 17-03-2017', track: { color: 'green', fontColor: 'green', id: 0, name: 'Track 1' }, microlocation: { id: 0, name: 'Room 1' }, speakers: [{ shortBiography: 'Works for ORG 1', id: 0, city: 'Delhi', name: 'Arnold Singh', speakingExperience: 'GSOC 2015', organisation: 'ORG 1', longBiography: '', photo: { 'iconImageUrl': '' } }] },


Like I said in the issue, the startsAt and endsAt fields will be a js Date object. not a string.

@niranjan94 As we are going to use model for getting sessions, it would be better if we use parse the dates in the model.

The model's output is gonna be a js date object. (It will parse the API Response and give a js date output) ... And we shouldn't do the date -> string conversion in the model ... because the string date format we require might vary depending on where the session is being displayed in the UI.

Like for example,

in this page we may want to display it in a more human readable form like March 23, 2017 05:30 PM

In the sessions table we'll display 23/03/2017 05:30 PM

In the form we may display in another way for it to work with the date picker.

We can't and shouldn't put all of this inside the model.

Okay got it!

niranjan94 · 2017-05-22T05:38:37Z

app/helpers/underscore-text.js

+
+const { Helper } = Ember;
+
+export function underscoreText(params) {


Please remove this helper and use the underscore helper from ember-cli-string-helpers which is already a part of this project.

niranjan94 · 2017-05-22T05:39:56Z

app/templates/components/public/session-filter.hbs

@@ -0,0 +1,5 @@
+<div class="ui segment">
+  {{#each-in tracks as |track|}}


I think tabs would look better. And when a user presses on a track, don't scroll to it. Instead filter the whole list to display only that track.

dilpreetsio · 2017-05-23T05:35:12Z

@niranjan94 Please review.

niranjan94

Be reactive to data. Let computed properties do a lot of heavy lifting for you.

In your action based implementation, even if I'm on all tracks and keep pressing the all tab, the whole action will keep getting executed and hence the sessionsByTracks will keep getting computed.

But in the case of computed properties, unless the selected track changes, it'll not get recomputed. You can click on a button how many ever times you want.

And the next thing is, when you have an ID for any data object, never depend on it's name.

niranjan94 · 2017-05-23T05:48:28Z

app/controllers/public/sessions.js

+    return keys(groupBy(orderBy(this.get('sessions'), 'startAt'), 'track.name'));
+  }),
+  actions: {
+    filterSession(element) {


Remove this action.

niranjan94 · 2017-05-23T05:49:49Z

app/controllers/public/sessions.js

+  sessions: [{ shortAbstract: 'An introduction to the event', id: '1', title: 'Welcome to FOSSASIA', startAt: moment(new Date()).format('h:mm a / DD-MM-YYYY'), track: { color: 'green', fontColor: 'green', id: 0, name: 'Track 1' }, microlocation: { id: 2, name: 'Room 1' }, speakers: [{ shortBiography: 'Works for ORG 1', id: 0, city: 'Delhi', name: 'Arnold Singh', speakingExperience: 'GSOC 2015', organisation: 'ORG 1', longBiography: '', photo: { 'iconImageUrl': '' } }] },
+  { shortAbstract: 'Welcome to open source', id: '2', title: 'Introduction to Open source', startAt: moment(new Date().setMinutes(10)).format('h:mm a / DD-MM-YYYY'), track: { color: 'red', fontColor: 'white', id: 0, name: 'Track 2' }, microlocation: { id: 3, name: 'Room 5' }, speakers: [{ shortBiography: '', id: 0, city: 'Delhi', name: 'Tux', speakingExperience: 'Speaker at the high conference', organisation: 'ORG 2', longBiography: '', photo: { 'iconImageUrl': '' } }] }
+  ],
+  sessionsByTracks: computed('sessions.[]', function() {


Let this computed function depend on sessions.[] and selectedTrackId

You don't have to declare selectedTrackId anywhere. it is by default null. If it is null consider it to be all tracks. Else filter by that track id

niranjan94 · 2017-05-23T05:50:43Z

app/templates/public/sessions.hbs

@@ -1 +1,4 @@
-{{outlet}}
+<h2 class="ui header">Sessions</h2>
+{{public/session-filter filterSession=(action 'filterSession') tracks=tracks}}


Change to

{{public/session-filter selectedTrackId=selectedTrackId tracks=tracks}}

Note that you're passing in the selectedTrackId variable

niranjan94 · 2017-05-23T05:51:20Z

app/components/public/session-filter.js

+
+  actions: {
+    filter(element, event) {
+      this.set('selectedTrack', event.target.id);


Change to

this.set('selectedTrackId', trackId);

niranjan94 · 2017-05-23T05:51:34Z

app/components/public/session-filter.js

+  selectedTrack: 'all',
+
+  actions: {
+    filter(element, event) {


Change to

filter(trackId = null) {

If no trackId is passed it is considered as null

niranjan94 · 2017-05-23T05:53:46Z

app/templates/components/public/session-filter.hbs

@@ -0,0 +1,10 @@
+<div class="ui secondary menu">
+  <a href="#" class="item {{if (eq selectedTrack 'all') 'active'}}" id="all" onclick={{action 'filter' 'All'}}>


Change to

<a href="#" class="item {{unless selectedTrackId 'active'}}" {{action 'filter'}}>

Here if selectedTrackId is null it is considered to be all tracks.
And for action you don't have to specify onclick=
And note that we're not passing any argument to the action. hence trackId in the action will be null.

niranjan94 · 2017-05-23T05:54:28Z

app/templates/components/public/session-filter.hbs

+    All
+  </a>
+  {{#each tracks as |track|}}
+    <a href="#" class="item {{if (eq selectedTrack (underscore track)) 'active'}}" id="{{underscore track}}" onclick={{action 'filter' track}}>


Can be changed to

<a href="#" class="item {{if (eq selectedTrackId track.id) 'active'}}" {{action 'filter' track.id}}> {{track.name}} </a>

niranjan94 · 2017-05-23T06:02:29Z

Now from anywhere in that page, all I have to do is change selectedTrackId and everything related to it will change.

dilpreetsio · 2017-05-23T09:00:42Z

@niranjan94 Thank you for the detailed review and proposing a better solution. I have made the required changes. Please review it.

niranjan94

@geekyd

niranjan94 · 2017-05-23T09:17:41Z

app/controllers/public/sessions.js

+    }
+  }),
+  tracks: computed('sessions.[]', function() {
+    var allTracks = mapValues(keyBy(orderBy(this.get('sessions'), 'startAt'), 'track.id'), function(v) {


This can be better written as

chain(this.get('sessions')) .map('track') .uniqBy('id') .orderBy('name') .value()

chain is also a lodash method.

That way we don't have to order all the sessions before getting the tracks and we get all contents of the track object.

The output will be an array of track objects.

niranjan94 · 2017-05-23T09:26:21Z

app/controllers/public/sessions.js

+  { shortAbstract: 'Welcome to open source', id: '2', title: 'Introduction to Open source', startAt: moment(new Date().setMinutes(10)).format('h:mm a / DD-MM-YYYY'), track: { color: 'red', fontColor: 'white', id: 2, name: 'Track 2' }, microlocation: { id: 3, name: 'Room 5' }, speakers: [{ shortBiography: '', id: 0, city: 'Delhi', name: 'Tux', speakingExperience: 'Speaker at the high conference', organisation: 'ORG 2', longBiography: '', photo: { 'iconImageUrl': '' } }] }
+  ],
+  sessionsByTracks: computed('sessions.[]', 'selectedTrackId', function() {
+    var allSessions = mapValues(groupBy(orderBy(this.get('sessions'), 'startAt'), 'track.id'), function(v) {


This can be written as

Unfiltered:

chain(this.get('sessions')) .orderBy(['track.name', 'startAt']) .groupBy('track.name') .value();

Filtered:

chain(this.get('sessions')) .filter(['track.id', this.get('selectedTrackId')]) .orderBy(['track.name', 'startAt']) .groupBy('track.name') .value();

Ordering and grouping are the operations that usually take up more time. So all filtering should be done before them so that they get as less data as possible.

This will give an output like

{ 'Track name': [] }

Where [] is an array of sessions. You can use each-in in the template to loop through this and get both track name and the sessions.

niranjan94 · 2017-05-23T09:29:44Z

app/controllers/public/sessions.js

+const { Controller, computed } = Ember;
+
+export default Controller.extend({
+  sessions: [{ shortAbstract: 'An introduction to the event', id: '1', title: 'Welcome to FOSSASIA', startAt: moment(new Date()).format('h:mm a / DD-MM-YYYY'), track: { color: 'green', fontColor: 'green', id: 1, name: 'Track 1' }, microlocation: { id: 2, name: 'Room 1' }, speakers: [{ shortBiography: 'Works for ORG 1', id: 0, city: 'Delhi', name: 'Arnold Singh', speakingExperience: 'GSOC 2015', organisation: 'ORG 1', longBiography: '', photo: { 'iconImageUrl': '' } }] },


Again ... like I said startAt and endAt will NOT be a string. It will be a date object. You need to use ember-moment template helpers to display it how you like.

dilpreetsio · 2017-05-23T15:02:54Z

@niranjan94 Please review.

niranjan94 · 2017-05-23T15:03:58Z

app/controllers/public/sessions.js

+    var allSessions = chain(this.get('sessions'))
+      .orderBy(['startAt', 'startAt'])
+      .groupBy('track.id')
+      .mapValues(function(v) {


Why is this needed ?

Since you're anyway only using track name in the view.

Just a groupBy track.name should give you all the info you need+sessions

I'm using the id to map the tracks and not the names anymore. That's the reason we are using this function to map the track id with the sessions.

You're performing unnecessary operations here. With what I suggested here > #68 (comment) you can perform one operation less and yet achieve the same results.

For UI grouping, track name shouldn't be an issue.

We pass tracks to the filter component which stores selectedTrackId the id of the selected track. We need to update the session-filter according to this information, therefore I have used track id and not track name to populate and bind the sessions under a track id to the id in session-filter.

Now whenever selectedTrackId changes it filters the sessions of session-list according to the trackid.

@geekyd that's exactly what is being done here too #68 (comment) but with one less operation.

With one or two sessions, a single less operation isn't gonna matter. But with hundreds of session, a single operation, especially ones like mapValues matter a lot

niranjan94 · 2017-05-23T15:05:08Z

app/controllers/public/sessions.js

+  ],
+  sessionsByTracks: computed('sessions.[]', 'selectedTrackId', function() {
+    var allSessions = chain(this.get('sessions'))
+      .orderBy(['startAt', 'startAt'])


Duplicate entry

dilpreetsio · 2017-05-23T15:31:26Z

@niranjan94 Removed mapValues please check.

niranjan94 · 2017-05-23T15:33:43Z

app/templates/components/public/session-list.hbs

@@ -0,0 +1,8 @@
+{{#each-in sessions as |id trackSession|}}


call id as trackName

niranjan94 · 2017-05-23T15:35:13Z

app/templates/components/public/session-list.hbs

@@ -0,0 +1,8 @@
+{{#each-in sessions as |id trackSession|}}


Now. When it is filtered by a track, what is going to come here is an array and not an object. Please handle that too.

TL,DR;
The session-list component should be able to render both an array of sessions and an object (when it is grouped by track)

You can use the is-array helper available

There is no need to handle this as output of both filter and unfiltered data is always going to be an object and not an array.
The format:
{ track1: Array, track2: Array ...}

Crap. Did not notice that groupBy in the filtered output. We should remove that. No point grouping by track when we are filtering by track.

And one more reason to have array support. If at in future we implement API calls for session filtering, we'll be getting sessions as array in the response.

dilpreetsio · 2017-05-23T16:43:34Z

@niranjan94 Made the changes. Please review.

niranjan94

@geekyd

niranjan94 · 2017-05-23T18:29:09Z

app/templates/components/public/session-list.hbs

+    {{/each}}
+  {{else}}
+    {{#each-in sessions as |track trackSession|}}
+      <h3 class="ui header" id="{{underscore track}}">{{track}}</h3>


The ID can be removed

niranjan94 · 2017-05-23T18:29:13Z

app/templates/components/public/session-list.hbs

@@ -0,0 +1,15 @@
+<div class="ui session-container">
+  {{#if (is-array sessions)}}
+    <h3 class="ui header" id="{{underscore track}}">{{track}}</h3>


The ID can be removed

niranjan94 · 2017-05-23T18:30:38Z

app/components/public/session-item.js

+const { Component } = Ember;
+
+export default Component.extend({
+  classNames: ['ui', 'accordion', 'segment']


The accordion class can be removed here. This will fix the extra space on top.

niranjan94 · 2017-05-23T18:31:15Z

app/templates/components/public/session-item.hbs

+  <div class="title">
+    <div class="ui">
+      <h4 class="ui header">{{session.title}}</h4>
+      <div class="ui row">Talk</div>


This should be the session type.

And add this as a subheader to your header for a better UI

<h4 class="ui header"> {{session.title}} <div class="sub header"> {{session.sessionType.name}} </div> </h4>

Also, increase from h4 to h3

niranjan94 · 2017-05-23T18:37:49Z

app/templates/components/public/session-item.hbs

+    </div>
+    <br>
+    <div class="ui grid">
+      <div class="column twelve wide">


Let the speaker image, microlocation name and start time be in one row and the session shortAbstract in a different row. That would fix the sessionAbstract speaker image spacing issue

niranjan94 · 2017-05-23T18:38:28Z

app/templates/components/public/session-item.hbs

+  <div class="title">
+    <div class="ui">
+      <h4 class="ui header">{{session.title}}</h4>
+      <div class="ui row">Talk</div>


Also, increase from h4 to h3

dilpreetsio · 2017-05-23T19:24:06Z

@niranjan94 Please review.

niranjan94 · 2017-05-24T04:14:06Z

app/templates/components/public/session-item.hbs

+      </div>
+    </div>
+    <div class="ui row">
+      <div class="session-description">


There should be a column inside the row. So put this inside a div with class column

dilpreetsio · 2017-05-24T04:32:20Z

@niranjan94 Made the changes. Please review.

niranjan94 · 2017-05-24T05:16:28Z

app/templates/components/public/session-item.hbs

+        <div class="">{{moment-format session.startAt 'hh:mm a / DD-MM-YYYY'}} <i class="wait icon"></i></div>
+      </div>
+    </div>
+    <div class="ui row">


@geekyd

Looks like the row is out side the grid ?

Should be something like this.

<div class="ui grid"> <div class="row">  </div> <div class="row"> <div class="column"> {{session.shortAbstract}} </div> </div>

A row cannot exist outside a ui grid

dilpreetsio · 2017-05-24T06:26:16Z

@niranjan94 Please review.

niranjan94 temporarily deployed to eventyay-frontend-pr-68 May 22, 2017 05:05 Inactive

niranjan94 suggested changes May 22, 2017

View reviewed changes

niranjan94 force-pushed the development branch 2 times, most recently from f03e8d6 to d35dad4 Compare May 22, 2017 08:12

dilpreetsio force-pushed the development branch from 249d02e to 61bcd10 Compare May 23, 2017 05:26

niranjan94 suggested changes May 23, 2017

View reviewed changes

dilpreetsio force-pushed the development branch from 61bcd10 to b816d68 Compare May 23, 2017 08:59

niranjan94 suggested changes May 23, 2017

View reviewed changes

dilpreetsio force-pushed the development branch from b816d68 to 049eb97 Compare May 23, 2017 14:50

niranjan94 suggested changes May 23, 2017

View reviewed changes

dilpreetsio force-pushed the development branch from 049eb97 to 45b942c Compare May 23, 2017 15:28

niranjan94 suggested changes May 23, 2017

View reviewed changes

dilpreetsio force-pushed the development branch from 45b942c to 0f799a4 Compare May 23, 2017 16:42

niranjan94 temporarily deployed to eventyay-frontend-pr-68 May 23, 2017 18:22 Inactive

niranjan94 suggested changes May 23, 2017

View reviewed changes

dilpreetsio force-pushed the development branch from 0f799a4 to a60317f Compare May 23, 2017 19:13

niranjan94 suggested changes May 24, 2017

View reviewed changes

dilpreetsio force-pushed the development branch from a60317f to 79076c5 Compare May 24, 2017 04:19

niranjan94 temporarily deployed to eventyay-frontend-pr-68 May 24, 2017 04:50 Inactive

niranjan94 suggested changes May 24, 2017

View reviewed changes

Adds sessions layout public event

9c48c91

dilpreetsio force-pushed the development branch from 79076c5 to 9c48c91 Compare May 24, 2017 06:01

niranjan94 approved these changes May 24, 2017

View reviewed changes

niranjan94 merged commit b5f5a9e into fossasia:development May 24, 2017


		const { Helper } = Ember;

		export function underscoreText(params) {

		@@ -0,0 +1,5 @@
		<div class="ui segment">
		{{#each-in tracks as \|track\|}}

		@@ -0,0 +1,10 @@
		<div class="ui secondary menu">
		<a href="#" class="item {{if (eq selectedTrack 'all') 'active'}}" id="all" onclick={{action 'filter' 'All'}}>

Adds sessions layout public event #68

Adds sessions layout public event #68

Conversation

dilpreetsio commented May 22, 2017

Short description of what this resolves:

codecov bot commented May 22, 2017 • edited

Codecov Report

lunaticmonk commented May 22, 2017

niranjan94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niranjan94 May 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilpreetsio commented May 23, 2017 • edited

niranjan94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niranjan94 commented May 23, 2017 • edited

dilpreetsio commented May 23, 2017

niranjan94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niranjan94 May 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilpreetsio commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilpreetsio May 23, 2017 • edited

Choose a reason for hiding this comment

niranjan94 May 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilpreetsio commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilpreetsio commented May 23, 2017

niranjan94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilpreetsio commented May 23, 2017

Choose a reason for hiding this comment

dilpreetsio commented May 24, 2017

Choose a reason for hiding this comment

dilpreetsio commented May 24, 2017

codecov bot commented May 22, 2017 •

edited

niranjan94 May 22, 2017 •

edited

dilpreetsio commented May 23, 2017 •

edited

niranjan94 commented May 23, 2017 •

edited

niranjan94 May 23, 2017 •

edited

dilpreetsio May 23, 2017 •

edited

niranjan94 May 23, 2017 •

edited