feat(MdTable): Implement an asynchronous table #1621

emman27 · 2018-03-23T03:27:52Z

An initial implementation of a table with data hosted on a JSON endpoint.

Fetches from endpoint
Allows using a nested data structure
Allows custom headers
Allows custom parameters in the querystring of the request
Link up with pagination

VdustR · 2018-03-23T03:53:54Z

package.json

-  "dependencies": {},
+  "dependencies": {
+    "axios": "^0.18.0"
+  },


why axios changed to dependencies?

Could potentially just use native fetch, tho fetch isn't supported in all browsers. Any other preferences?

I mean all modules imported are in devDependencies for now.

Hmm. I think if there is going to be to support a http client natively to this project, then the client used internally needs to be either a dependency or a peer dependency. Adding as dev dependency will eventually result in it not being installed by npm install vue-material, which may cause confusion for users.

Also open to replacing this with XMLHttpRequest (preferrably not, since this has really fallen out) or fetch (with a polyfill installed as dependency), depending on your team's preference for direction

You don't need to install the modules which vue-material used because the vue-material module you importing is production that already compiled with all modules imported to it.

For example, you can import vue-material without problem if you don't install date-fns and popper.js.

VdustR · 2018-03-23T03:58:46Z

src/components/MdTable/MdTable.vue

+      },
+      asyncHeaders: {
+        type: Object,
+        default: () => {}


All vue-material props should be started with prefix md

VdustR · 2018-03-23T04:19:30Z

src/components/MdTable/MdTable.vue

@@ -296,6 +312,10 @@
      // wait for `selectingMode` from `TableRow`
      await this.$nextTick()
      this.syncSelectedValue()
+      if (this.asyncUrl) {
+        const data = (await axios.get(this.asyncUrl, { params: this.asyncParams, headers: this.asyncHeaders })).data


Should there be a loading UI while requesting?
How about a named slot with indeterminate loading bar as default?

VdustR · 2018-03-23T05:21:13Z

@emman27 check list markdown is - [ ], - [x]

VdustR · 2018-03-23T05:26:12Z

src/components/MdTable/MdTable.vue

@@ -45,6 +45,9 @@
      <slot name="md-table-pagination" />
    </md-content>

+    <slot name="md-table-loading"></slot>


only if asyncLoading

VdustR · 2018-03-23T05:27:17Z

src/components/MdTable/MdTable.vue

@@ -45,6 +45,9 @@
      <slot name="md-table-pagination" />
    </md-content>

+    <slot name="md-table-loading"></slot>
+    <md-progress-bar md-mode="indeterminate" v-if="!$scopedSlots['md-table-loading'] && asyncLoading" />
+


How about this?

<slot name="md-table-loading" v-if="asyncLoading"> <md-progress-bar md-mode="indeterminate" /> </slot>

Thanks for helping me out with this! Not extremely familiar with slots myself.

VdustR · 2018-03-23T06:02:52Z

Hey @emman27, would you resolve conflicts? Actually it's better to start from dev branch to make a PR.

VdustR · 2018-03-23T06:15:47Z

You need to fetch current dev before you merge that.

emman27 · 2018-03-23T06:18:26Z

Thanks, was a careless mistake and pushed too early. Fixed now.

VdustR · 2018-03-23T07:43:36Z

Although this is almost completed, I think make v-model like md-options as a Promise in MdAutocomplete might be better for most cases.

Samuell1 · 2018-03-23T07:48:20Z

Thanks for your first PR! First i think axios should not be a package that we depends on. Best to make it lightweight is to make a callback like md-autocomplete is using.

And please dont use await, async it will broke code - #1182

(If you will be making next PR please follow this guide how to name commits, it will helps us to identify changes + make changelog. https://github.com/vuematerial/vue-material/blob/dev/.github/COMMIT_CONVENTION.md )

emman27 · 2018-03-23T09:33:56Z

Thanks guys, really appreciate what you've built here plus your feedback!

Samuell1 · 2018-03-23T10:44:03Z

@emman27 Thanks you too for contributing and using Vue Material!

I found there are 2 methods (getModel, getModelItem) that are still pointing to old this.value

And last thing dont forget in that commits naming : after feat(...)

Samuell1 · 2018-03-23T10:46:26Z

src/components/MdTable/MdTable.vue

@@ -357,13 +342,14 @@
    },
    async created () {


I think you forgot to remove async here

Samuell1 · 2018-03-23T10:57:58Z

src/components/MdTable/MdTable.vue

@@ -153,7 +158,9 @@
          getModel: this.getModel,
          getModelItem: this.getModelItem,
          selectingMode: null
-        }
+        },
+        items: this.value || [],


I think this should be as Computed

Hmm I think I'll disagree with this one. This is because we'll have to set the value once the Promise resolves.
Vue recommends not modifying props directly and instead using a data property, or computed if possible. Since there's a set involved, shouldn't data be the more natural choice?

Yes but when i pass my array it will be not reactive to changes because data are changed only on mounted.

And there are few things that you forgot to change 2 methods (getModel, getModelItem) that are still pointing to old this.value

Hmm good point. I believe this can be resolved by

watch: { value() {} }

as well. Which do you prefer?

Yeah, watch should be good

…items in all cases

Samuell1 · 2018-03-27T11:48:06Z

src/components/MdTable/MdTable.vue

@@ -215,6 +215,7 @@
      return { MdTable }
    },
    watch: {
+      value() {}, // Watch this prop to make the table reactive to props


This is not finished? You missing here to set value as items.

Hey you're right. Now I understand why you suggested computed instead, would probably be cleaner. My apologies. Didn't consider the Promise case :(

…ew promises

Samuell1 · 2018-03-28T06:43:09Z

src/components/MdTable/MdTable.vue

@@ -215,7 +215,9 @@
      return { MdTable }
    },
    watch: {
-      value() {}, // Watch this prop to make the table reactive to props
+      value() {
+        if (isPromise(this.value)) this.resolvePromise()


you need to implement else too because all other tables that doesnt use promise are broken.

I think better is to make it as computed method, then you can remove items defined in data.

items () { if (isPromise(this.value)) { return this.resolvePromise() } else { return this.value } }

There shouldn't be a mutation in a computed.

Thanks guys. Care to share the secret of how you guys are so sharp?

@Samuell1 I think that example doesn't work because this.resolvePromise doesn't (and cannot?) return the resolved value.

@emman27 It will work like then((resolve) => (resolve)), but @VdustR said it will be needed to change resolvePromise method.

I think this will now work.

asadhshujau · 2018-04-25T09:06:02Z

when will we get pagination feature. waiting for it ...

dragosct · 2020-04-21T08:23:52Z

Hi, @emman27! Thanks for your work. Can you please resolve the conflicts to merge this PR?

emman27 · 2020-06-10T12:30:42Z

Hi, @emman27! Thanks for your work. Can you please resolve the conflicts to merge this PR?

Hi @dragosct10, unfortunately I don't quite have the time to work on this these days. I hope the code is still helpful for you guys and feel free to adopt it.

Implement an asynchronous table

a47635c

VdustR reviewed Mar 23, 2018

View reviewed changes

Add Md prefix to props and add an indeterminate loading bar to Table

c4eee33

VdustR reviewed Mar 23, 2018

View reviewed changes

Emmanuel Goh added 3 commits March 23, 2018 13:30

Clean up code for md-table-loading

1840b3c

Move axios back to dev dependencies

10c142f

Remove accidentally added style tags

5c99b07

emman27 force-pushed the async-tables branch from 00164d3 to 322993b Compare March 23, 2018 06:12

Merge remote-tracking branch 'original/dev' into async-tables

98260ed

emman27 force-pushed the async-tables branch from 322993b to 98260ed Compare March 23, 2018 06:17

emman27 changed the title ~~Implement an asynchronous table~~ feat(MdTable) Implement an asynchronous table Mar 23, 2018

feat(MdTable) Move to promise based instead of url

bf7f83d

Samuell1 reviewed Mar 23, 2018

View reviewed changes

src/components/MdTable/MdTable.vue Outdated

@@ -357,13 +342,14 @@

},

async created () {

Copy link

Member

Samuell1 Mar 23, 2018

Choose a reason for hiding this comment

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

I think you forgot to remove async here

Samuell1 reviewed Mar 23, 2018

View reviewed changes

feat(MdTable): Remove unnecessary async method

3280086

emman27 changed the title ~~feat(MdTable) Implement an asynchronous table~~ feat(MdTable): Implement an asynchronous table Mar 26, 2018

feat(MdTable): Make props reactive to changes and replace value with …

24d1ffd

…items in all cases

Samuell1 reviewed Mar 27, 2018

View reviewed changes

feat(MdTable): Fix table not correctly reacting to props changes to n…

ee210f8

…ew promises

Samuell1 reviewed Mar 28, 2018

View reviewed changes

feat(MdTable): Fix setting of items for non-promise based tables

8a03321

jordigoyanes approved these changes Apr 21, 2018

View reviewed changes

marqbeniamin added C: MdTable discussion in review labels Nov 27, 2019

marqbeniamin closed this Feb 26, 2021

feat(MdTable): Implement an asynchronous table #1621

feat(MdTable): Implement an asynchronous table #1621

Conversation

emman27 commented Mar 23, 2018 • edited by marcosmoura Loading

Choose a reason for hiding this comment

emman27 Mar 23, 2018 • 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

Choose a reason for hiding this comment

VdustR commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VdustR commented Mar 23, 2018

VdustR commented Mar 23, 2018

emman27 commented Mar 23, 2018

VdustR commented Mar 23, 2018

Samuell1 commented Mar 23, 2018 • edited Loading

emman27 commented Mar 23, 2018

Samuell1 commented Mar 23, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Samuell1 Mar 28, 2018 • 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

asadhshujau commented Apr 25, 2018

dragosct commented Apr 21, 2020

emman27 commented Jun 10, 2020

emman27 commented Mar 23, 2018 •

edited by marcosmoura

Loading

emman27 Mar 23, 2018 •

edited

Loading

Samuell1 commented Mar 23, 2018 •

edited

Loading

Samuell1 commented Mar 23, 2018 •

edited

Loading

Samuell1 Mar 28, 2018 •

edited

Loading