Skip to content

Conversation

@abhinavk96
Copy link
Contributor

@abhinavk96 abhinavk96 commented Jun 22, 2019

@niranjan94 I am in the midst of implementing this, I need feedback on certain conventions.

1 > Is this the right way to reopen and initialize the table?

2> If not, is there any reference for it other than API docs.

3> Actions. What would be a good way to pass them, while keeping the layout general still.

4> Please ignore pagination boiler-plate for now.

5 > Pagination | Should it be inside tfoot? Or outside the table

6 > Search bar | Should it be in thead ? Or outside the table tag
Preview so far:

image

@abhinavk96 abhinavk96 requested a review from niranjan94 June 22, 2019 06:38
@@ -0,0 +1,16 @@
{{#ember-table class="ui table" as |t|}}
Copy link
Member

Choose a reason for hiding this comment

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

No need to pass in the class ui table here

package.json Outdated
"@bower_components/tinyColorPicker": "PitPik/tinyColorPicker#^1.1.1",
"@bower_components/wysihtml": "Voog/wysihtml#^0.5.5"
"@bower_components/wysihtml": "Voog/wysihtml#^0.5.5",
"ember-table": "addepar/ember-table#0aa5637"
Copy link
Member

Choose a reason for hiding this comment

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

Move this to devDependencies

@niranjan94
Copy link
Member

niranjan94 commented Jun 22, 2019

I am in the midst of implementing this, I need feedback on certain conventions.

  1. What you're doing is fine.
  2. Which actions are we talking about ?
  3. Sure. Ignored.
  4. Outside the table.
  5. Outside the table.

@abhinavk96
Copy link
Contributor Author

abhinavk96 commented Jun 22, 2019

Which actions are we talking about?

@niranjan94 Actions which are defined inside the controller but are triggered from buttons inside individual cell components. We were using closure actions, so they were passed from the route template to the table. Components directly made use of them. However, they didn't need to be explicitly passed to the cell component, passing them, to the table was enough. It's not true in this case. Passing all the actions to each commponent cell seems like overkill.

@@ -0,0 +1,16 @@
{{#ember-table class="ui table" as |t|}}
{{t.head columns=columns}}
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to put all of this boilerplate code for every table we want to render. So move this out to a component. And then call that component here with the rows, columns.

@niranjan94
Copy link
Member

For the actions ...

Let me try something out locally and get back to you.

@abhinavk96
Copy link
Contributor Author

abhinavk96 commented Jun 22, 2019 via email

@abhinavk96
Copy link
Contributor Author

abhinavk96 commented Jun 22, 2019

@niranjan94 For pagination, should we follow a queryParams approach? Wherein I update the query param with refreshModel true so that new data gets loaded into a table?

Going ahead with it at the moment, load speeds are much better than ember-model tables.

@abhinavk96
Copy link
Contributor Author

@niranjan94 I have completed the pagination functionality, both logic and template, please review that portion. I will work on sorting, search next.

@fossasia fossasia deleted a comment Jun 25, 2019
@fossasia fossasia deleted a comment Jun 25, 2019
import Component from '@ember/component';
import { A } from '@ember/array';

export default Component.extend({
Copy link
Member

Choose a reason for hiding this comment

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

Lets start using ES6 classes + decorators for all new resources. That's the way ember is moving towards.

import { A } from '@ember/array';

export default class extends Component {
pageSizes = A([10, 25, 50, 100, 250, 'All']);
Copy link
Member

Choose a reason for hiding this comment

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

A not needed. Stick to native arrays whenever possible.

@@ -0,0 +1,4 @@
import Component from '@ember/component';

export default Component.extend({
Copy link
Member

Choose a reason for hiding this comment

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

ES6 class

}

@action
doAction(eventName) {
Copy link
Member

Choose a reason for hiding this comment

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

Can remove

@abhinavk96
Copy link
Contributor Author

@niranjan94 From what I gather, if we don't want to use custom css classes, we want this:Semantic-Org/Semantic-UI#2901
For the individual columns of the table to not be resizable with screen size, the x-scrollable method is not working with ember-tables, please let me know how we should proceed.

@kushthedude
Copy link
Member

@kushthedude I don't want to make the table basic and even then, the ember-table properties are interfering with it, and the column contents are getting squashed into each other. Also, innerTableWrapper was defined in ember-model-table theme, that class has no impact here.

I got that, Even the columns are disappearing and collapsing for mobile view, Let's find some new way

@kushthedude
Copy link
Member

@CosmicCoder96 Isn't there some attribute like row-span or column-span in the ember-table as we used to have in ember-models table?

niranjan94
niranjan94 previously approved these changes Jul 4, 2019
Copy link
Member

@niranjan94 niranjan94 left a comment

Choose a reason for hiding this comment

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

I'm okay with merging this in at this state. This is big change and has already improved the tables experience many times more than ember-model-tables.

We'll merge this in right now so that we can start using it on other pages asap.

@fossasia fossasia deleted a comment Jul 4, 2019
Copy link
Member

@kushthedude kushthedude left a comment

Choose a reason for hiding this comment

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

Let's use Ember Tables then.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants