[MOD-136][Improvement] Loading for the dashboard #73

laurenbarker · 2017-10-23T19:50:11Z

TODO

Use the cos-fork of ember-content-placeholders

Changes

Install ember-concurrency
Move loading data out of routes
Add skeleton screens for dashboard

Use skeleton screen on action-feed

aaxelb

Overall, looks awesome, just a smattering of small things.

Tiny bug: I think something happened moving data out of the route -- when I refresh the settings page, the count on the moderation tab is initially correct, then changes to 0 after a second.

...and there were some tests merged earlier today that this breaks, sorry about that.

aaxelb · 2017-11-06T21:10:16Z

app/components/action-feed/template.hbs

@@ -1,14 +1,18 @@
-{{#if listLoading}}
-    {{loading-indicator classes='ball-dark text-center loading'}}
+{{#if (and loadActions.isRunning (not loadingMore))}}


I think we could get rid of loadingMore and DRY this up a little...

{{#each actionsList ...}} entry {{else if (not loadActions.isRunning)}} No actions {{/if}} {{#if loadActions.isRunning}} dummy list {{else if moreActions}} see more {{/if}}

aaxelb · 2017-11-06T21:11:31Z

app/controllers/index.js

+    },
+
+    fetchData: task(function* () {
+        yield timeout(1000);


Is this left over from debugging?

aaxelb · 2017-11-06T21:11:52Z

app/routes/index.js

@@ -1,26 +1,4 @@
-import { inject as service } from '@ember/service';
 import Route from '@ember/routing/route';

 export default Route.extend({


Can we delete this file?

aaxelb · 2017-11-06T21:19:48Z

app/components/action-feed/component.js

+        const page = this.get('page');
+        try {
+            const results = yield this.get('currentUser.user')
+                .then(user => this.get('store').queryHasMany(user, 'actions', { page }));


This isn't directly related to your changes, but would it load faster if we added embed: 'target' here?

Embedding isn't faster but it prevents the piecemeal loading.

aaxelb · 2017-11-06T21:25:50Z

app/components/action-feed/component.js

+    loadActions: task(function* () {
+        const page = this.get('page');
+        try {
+            const results = yield this.get('currentUser.user')


Thoughts on the ember-concurrency idiom instead of chaining promises?

const user = yield this.get('currentUser.user'); const actions = yield this.get('store').queryHasMany(user, ...); this.get('actionsList').pushObjects(actions.toArray()); ...

aaxelb · 2017-11-06T21:28:03Z

app/components/action-feed/component.js

    nextPage() {
        this.incrementProperty('page');
        this.set('loadingMore', true);
-        this.loadPage();
+        this.get('loadActions').perform();


If you add .drop() to loadActions, seems like we could safely increment page inside it and get rid of nextPage

aaxelb · 2017-11-06T21:48:16Z

app/components/action-feed-entry/style.scss

@@ -1,10 +1,6 @@
 & {
  clear: both;

-  .entry-body {
-    padding: 20px 15px;


Why no padding?

Aw...I forgot about the hover box. The top of the list looks out of line with the provider box with top padding.

Remove extra file Chaining promises --> ember-concurrency Embed target DRY up code

coveralls · 2017-11-07T21:45:26Z

Coverage increased (+0.5%) to 14.6% when pulling 93e5de0 on laurenbarker:improvement/MOD-136 into d28e6cd on CenterForOpenScience:develop.

aaxelb

Found a little bug and had a few suggestions.

aaxelb · 2017-11-07T21:42:06Z

app/components/action-feed-entry-skeleton/template.hbs

+        <div class="icon--skeleton">{{placeholder.icon}}</div>
+    {{/content-placeholders}}
+    <div class="action-body--skeleton">
+        {{#content-placeholders as |placeholder|}}


Could these two {{#content-placeholders as |placeholder|}} blocks be combined in one?

aaxelb · 2017-11-07T21:45:28Z

app/components/landing-page/component.js

+            this.transitionToRoute('preprints.provider.preprint-detail', provider.get('id'), reviewable.get('id'));
+        },
+        setupProvider(provider) {
+            this.transitionToRoute('preprints.provider.setup', provider.id);


Uncaught TypeError: this.transitionToRoute is not a function

The router service would help.

aaxelb · 2017-11-07T21:49:07Z

app/components/landing-page/template.hbs

+    {{loading-indicator-page}}
+{{else}}
+    {{#if showDashboard}}
+        <div class='reviews-dashboard content'>


If we're doing all components, I think it'd be nice to make the landing page and dashboard separate components. Maybe the logic to switch between them would make sense on the controller? Not super important, but now seems like a convenient time to clean this up, if you don't mind...

The request for providers is what messes up the other pages. So I can move some stuff around but I don't think the logic to switch between them can go in the controller since it's called when the app refreshes no matter what page the user is on (that was the bug on the settings page).

aaxelb · 2017-11-07T21:53:54Z

app/components/action-feed/component.js

        this.setProperties({
-            totalPages: Math.ceil(response.get('links.meta.total') / response.get('links.meta.per_page')),
-            listLoading: false,
+            actionsList: [],
            loadingMore: false,


loadingMore is gone

Condense placeholders

coveralls · 2017-11-08T15:40:53Z

Coverage increased (+0.5%) to 14.6% when pulling 4f66212 on laurenbarker:improvement/MOD-136 into d28e6cd on CenterForOpenScience:develop.

aaxelb

Just one last thing, if it makes sense to you.

aaxelb · 2017-11-08T16:57:38Z

app/components/reviews-dashboard/template.hbs

+            </div>
+            <div class='col-md-8 col-md-pull-4'>
+                {{action-feed
+                    toDetail=(action transitionToDetail)


Since transitionToDetail is already a closure action, I think you could just do toDetail=transitionToDetail. But more than that, it feels wrong to pass this action so far down (controller => reviews-dashboard => action-feed => action-feed-entry) when action-feed-entry could just use the router service and trigger a transition all by itself.

I was trying to avoid that since it seems very anti-Ember but we are ignoring pretty much everything they recommend so I guess it doesn't really matter. I do hate passing things down 10 levels.

I think it'd make more sense if these components were meant to be reusable outside this app, or the action varied depending on other state, or it did anything other than a simple transition.

They added router, though, so I figure we may as well use it.

@aaxelb, bad news. We can't use the router service yet because their is a bug that breaks when all query params aren't present. So when we try and use this.get('router').transitionTo(...) we always get an error about the ticket query parameter on the application route.

"Assertion Failed: You passed the `false` query parameter during a transition into application, please update to ticket"

Fix release:

Apologies for not getting this in for 2.16, I'll get the changes into the next 2.17 beta release and once some folks can confirm all is working well for us I'll pull down into 2.16 for the LTS.

That fix is in 2.16.1... we should upgrade. In another ticket.

Oh nice, I missed that. I'll add a ticket 👍

aaxelb · 2017-11-08T17:03:48Z

app/components/landing-page/template.hbs

+            <div class="text-center m-t-lg col-xs-12">
+                <div class="m-b-lg reviews-brand"></div>
+                {{#if showSetup}}
+                    {{start-moderating action=(action setupProvider) providers=providersToSetup}}


Same thing here: It seems cleaner for start-moderating to use the router service and trigger the transition itself, rather than passing setupProvider all the way down from the controller.

laurenbarker force-pushed the improvement/MOD-136 branch from ad2c1e8 to 0e16f50 Compare October 24, 2017 19:41

laurenbarker force-pushed the improvement/MOD-136 branch 2 times, most recently from c0a75ed to fce23bc Compare November 6, 2017 19:05

laurenbarker changed the title ~~[WIP][MOD-136][Improvement] Add loading pages~~ [WIP][MOD-136][Improvement] Loading for the dashboard Nov 6, 2017

laurenbarker force-pushed the improvement/MOD-136 branch from fce23bc to 5dc2f39 Compare November 6, 2017 19:08

laurenbarker added 2 commits November 6, 2017 14:09

Use tasks on dashboard

c582366

Use tasks on dashboard and action-feed

04b8a49

Use skeleton screen on action-feed

laurenbarker force-pushed the improvement/MOD-136 branch from 5dc2f39 to 04b8a49 Compare November 6, 2017 19:11

Use cos-forks version of emebr-content-placeholders

52d312f

laurenbarker changed the title ~~[WIP][MOD-136][Improvement] Loading for the dashboard~~ [MOD-136][Improvement] Loading for the dashboard Nov 6, 2017

aaxelb requested changes Nov 6, 2017

View reviewed changes

laurenbarker added 3 commits November 7, 2017 12:03

Fix action entry styling

453cb4a

Remove extra file Chaining promises --> ember-concurrency Embed target DRY up code

Move index.js to a component so it doesn't interfere with other pages

756b139

Fix tests

93e5de0

aaxelb requested changes Nov 7, 2017

View reviewed changes

Move landing page into separate components

ac4f336

laurenbarker force-pushed the improvement/MOD-136 branch from 14662e0 to 6468cef Compare November 8, 2017 15:35

Remove unused variables

4f66212

Condense placeholders

laurenbarker force-pushed the improvement/MOD-136 branch from 6468cef to 4f66212 Compare November 8, 2017 15:37

aaxelb requested changes Nov 8, 2017

View reviewed changes

aaxelb approved these changes Nov 8, 2017

View reviewed changes

aaxelb merged commit 1d150ff into CenterForOpenScience:develop Nov 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MOD-136][Improvement] Loading for the dashboard #73

[MOD-136][Improvement] Loading for the dashboard #73

laurenbarker commented Oct 23, 2017 •

edited

Loading

aaxelb left a comment

aaxelb Nov 6, 2017

aaxelb Nov 6, 2017

aaxelb Nov 6, 2017

aaxelb Nov 6, 2017

laurenbarker Nov 7, 2017

aaxelb Nov 6, 2017

aaxelb Nov 6, 2017

aaxelb Nov 6, 2017

laurenbarker Nov 6, 2017

coveralls commented Nov 7, 2017

aaxelb left a comment

aaxelb Nov 7, 2017

aaxelb Nov 7, 2017

aaxelb Nov 7, 2017

laurenbarker Nov 7, 2017

aaxelb Nov 7, 2017

coveralls commented Nov 8, 2017

aaxelb left a comment

aaxelb Nov 8, 2017

laurenbarker Nov 8, 2017 •

edited

Loading

aaxelb Nov 8, 2017

laurenbarker Nov 8, 2017

aaxelb Nov 8, 2017

laurenbarker Nov 8, 2017

aaxelb Nov 8, 2017

[MOD-136][Improvement] Loading for the dashboard #73

[MOD-136][Improvement] Loading for the dashboard #73

Conversation

laurenbarker commented Oct 23, 2017 • edited Loading

TODO

Changes

aaxelb 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

Choose a reason for hiding this comment

coveralls commented Nov 7, 2017

aaxelb 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

coveralls commented Nov 8, 2017

aaxelb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurenbarker Nov 8, 2017 • edited Loading

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

laurenbarker commented Oct 23, 2017 •

edited

Loading

laurenbarker Nov 8, 2017 •

edited

Loading