Skip to content
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

feat: switch to ember-table for all remaining tables [Admin Area] #3246

Merged
merged 8 commits into from
Jul 28, 2019

Conversation

abhinavk96
Copy link
Contributor

@abhinavk96 abhinavk96 commented Jul 5, 2019

Fixes #3169

Short description of what this resolves:

Replaces all the ember-model tables inside the admin area with ember-tables.

@auto-label auto-label bot added the feature label Jul 5, 2019
@abhinavk96 abhinavk96 changed the title feat: switch to ember-table for all tables feat: switch to ember-table for all remaining tables Jul 5, 2019
@fossasia fossasia deleted a comment Jul 6, 2019
app/controllers/admin/events/list.js Outdated Show resolved Hide resolved
app/controllers/admin/events/list.js Outdated Show resolved Hide resolved
app/controllers/admin/events/list.js Outdated Show resolved Hide resolved
app/controllers/admin/events/list.js Outdated Show resolved Hide resolved
app/controllers/admin/events/list.js Outdated Show resolved Hide resolved
app/controllers/admin/events/list.js Outdated Show resolved Hide resolved
app/controllers/admin/events/list.js Outdated Show resolved Hide resolved
app/controllers/admin/events/list.js Outdated Show resolved Hide resolved
const rows = [];
this.model.data.map(row => {
rows.pushObject({
name : row.get('event.name'),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@niranjan94
Events which share same session names:
image

app/controllers/admin/events/list.js Outdated Show resolved Hide resolved
app/controllers/admin/events/list.js Outdated Show resolved Hide resolved
app/controllers/admin/events/list.js Outdated Show resolved Hide resolved
@fossasia fossasia deleted a comment Jul 8, 2019
@abhinavk96 abhinavk96 changed the title feat: switch to ember-table for all remaining tables feat: switch to ember-table for all remaining tables [Admin Area] Jul 8, 2019
@fossasia fossasia deleted a comment Jul 8, 2019
@fossasia fossasia deleted a comment Jul 8, 2019
@abhinavk96
Copy link
Contributor Author

@niranjan94 It's ready for another review.

app/controllers/admin/reports/system-logs/activity-logs.js Outdated Show resolved Hide resolved
}

@computed('model.[]')
get rows() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use model directly. One less computed property. One less iteration of the dataset.

}

@computed('model.[]', 'model.@each.event')
get rows() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use model directly. One less computed property. One less iteration of the dataset.

app/controllers/admin/users/list.js Outdated Show resolved Hide resolved
app/controllers/admin/users/view/events/list.js Outdated Show resolved Hide resolved
@abhinavk96
Copy link
Contributor Author

Use model directly. One less computed property. One less iteration of the dataset.

@niranjan94 How exactly will that work?
model has the format:

image

where as the mapped array has the format:
image

Ember table expects the formatted mapped array?

@niranjan94
Copy link
Member

The models have getters for the properties already. So, even though you cannot see the property when you do a console.log, accessing them via for example, model.email would work ... And hence would work fine with ember tables too ... Just make sure the valuePath is correct for each column

@niranjan94
Copy link
Member

ember tables uses get internally ... So model.get() would work fine

@abhinavk96
Copy link
Contributor Author

abhinavk96 commented Jul 9, 2019

@niranjan94 I tried with a single column table,
my column definition schema being:

get columns() {
    return [
      {
        name      : 'Email',
        valuePath : 'email' 
     }
    ];
  }

passed rows=model in the template, model is identical to the one in the screenshot above. (user list)

It throws this exception:

collapse-tree.js:514 Uncaught (in promise) TypeError: Ember.get(...).some is not a function
    at Class.<anonymous> (collapse-tree.js:514)
    at ComputedProperty.get (metal.js:3200)
    at Object._get2 [as get] (metal.js:1614)
    at Class.<anonymous> (collapse-tree.js:609)
    at ComputedProperty.get (metal.js:3200)
    at _get2 (metal.js:1614)
    at _getPath (metal.js:1646)
    at Object._get2 [as get] (metal.js:1598)
    at Class.<anonymous> (collapse-tree.js:851)
    at ComputedProperty.get (metal.js:3200)

Collapsee tree.js:514:

return !Ember.get(this, 'value.children').some(function (child) {
        return Ember.isArray(Ember.get(child, 'children'));
      });

What else needs to change?

@fossasia fossasia deleted a comment Jul 12, 2019
app/mixins/ember-table-route.js Outdated Show resolved Hide resolved
app/controllers/admin/users/list.js Outdated Show resolved Hide resolved
@abhinavk96
Copy link
Contributor Author

@niranjan94 This is ready for review, I have used extraValue paths for all properties.

@@ -1,6 +1,7 @@
import Route from '@ember/routing/route';

export default Route.extend({
import { action } from '@ember/object';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CosmicCoder96 actually I'm already refactoring this file in my PR #3198 where I'm also introducing few session rating columns and actions. 😅
I'll introduce any changes if missing there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants