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

comp(mdTable): nextTick selection data #1874

Merged
merged 4 commits into from
Jul 26, 2018
Merged

Conversation

FossPrime
Copy link
Contributor

@FossPrime FossPrime commented Jul 24, 2018

The current implementation couldn't handle selection data and table data at the same time, or having selection data before table data #1866

Case 1: Update at same time
https://codesandbox.io/s/lp7117wylq

Case 2: Give selection AOT
https://codesandbox.io/s/6j7n78mz6z

docs/app/App.vue for case 2

<template>
  <div>
    <md-table v-model="people" md-card
        @md-selected="onSelect" :md-selected-value.sync="selected">
      <md-table-toolbar>
        <h1 class="md-title">With auto select and alternate headers</h1>
      </md-table-toolbar>

      <md-table-toolbar slot="md-table-alternate-header" slot-scope="{ count }">
        <div class="md-toolbar-section-start">{{ getAlternateLabel(count) }}</div>

        <div class="md-toolbar-section-end">
          <md-button class="md-icon-button">
            <md-icon>delete</md-icon>
          </md-button>
        </div>
      </md-table-toolbar>

      <md-table-row slot="md-table-row" slot-scope="{ item }" :md-disabled="item.name.includes('Stave')" md-selectable="multiple" md-auto-select>
        <md-table-cell md-label="Name" md-sort-by="name">{{ item.name }}</md-table-cell>
        <md-table-cell md-label="Email" md-sort-by="email">{{ item.email }}</md-table-cell>
        <md-table-cell md-label="Gender" md-sort-by="gender">{{ item.gender }}</md-table-cell>
        <md-table-cell md-label="Job Title" md-sort-by="title">{{ item.title }}</md-table-cell>
      </md-table-row>
    </md-table>

    <p>Selected:</p>
    {{ selected }}
  </div>
</template>

<script>
import '../../dist/vue-material.min.css'
import '../../dist/theme/default.css'

export default {
  name: "TableMultiple",
  data: () => ({
    selected: [],
    people: [],
    peopleServer: [
      {
        name: "Shawna Dubbin",
        email: "[email protected]",
        gender: "Male",
        title: "Assistant Media Planner"
      },
      {
        name: "Odette Demageard",
        email: "[email protected]",
        gender: "Female",
        title: "Account Coordinator"
      },
      {
        name: "Lonnie Izkovitz",
        email: "[email protected]",
        gender: "Female",
        title: "Operator"
      },
      {
        name: "Thatcher Stave",
        email: "[email protected]",
        gender: "Male",
        title: "Software Test Engineer III"
      },
      {
        name: "Clarinda Marieton",
        email: "[email protected]",
        gender: "Female",
        title: "Paralegal"
      }
    ]
  }),
  methods: {
    onSelect(items) {
      this.selected = items;
    },
    getAlternateLabel(count) {
      let plural = "";

      if (count > 1) {
        plural = "s";
      }

      return `${count} user${plural} selected`;
    }
  },
  created() {
    this.$set(this, "selected", [this.peopleServer[0]]);
    setInterval(async () => {
      this.$set(this, "people", this.peopleServer);
    }, 3000);
  }
};
</script>

<style lang="scss" scoped>
.md-table + .md-table {
  margin-top: 16px;
}
</style>

Ray Foss added 2 commits July 24, 2018 10:32
The current implementation couldn't handle selection data and table data at the same time vuematerial#1866
providing selection data Ahead of Time, aka, ahead of table data, will also break MdTable. This handles both cases.
@Samuell1
Copy link
Member

We cant use async/await because of #1182 . Do you think you can change that?

@@ -332,7 +335,8 @@
this.$emit('update:mdSelectedValue', val)
this.$emit('md-selected', val)
},
syncSelectedValue () {
syncSelectedValue: async function () {
await this.$nextTick() // render the table first
Copy link
Member

Choose a reason for hiding this comment

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

As @Samuell1 said #1874 (comment)
would you change it to this.$nextTick(() => { /* ... code here */ })?

Additionally, I think this $nextTick could be removed with this fix.
https://github.com/rayfoss/vue-mater/blob/42ce8fcc2ebf38986d9d09d5cf296ecce93212d3/src/components/MdTable/MdTable.vue#L357

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right about the created() -> nextTick(), it became superfluous and the bug fix still works on all cases without it.

@FossPrime
Copy link
Contributor Author

FossPrime commented Jul 24, 2018

https://github.com/nodejs/Release Node 6 is in maintenance mode... nothing new should be developed with it, everything old is version locked and won't be affected by non-patch updates. node 7.6 supports Async/await, and even that has been end-of-life since 2017-06-30

We should definitely be supporting async, as all current and active versions of Node support it. Otherwise, we're implying support for Node 6. We should probably enforce this in package.json engines... "node": ">= 7.0" Node 7 is, again end of life. This should be updated on the next minor update 1.1.0.

I saw a horrible tree of nested promises yesterday... I don't want to see those again

@marcosmoura
Copy link
Member

@rayfoss There is nothing related with Node version in this case. The problem with async/await is due to Babel code making our bundle way bigger than we need. For 1.0 we don't have any interests to support async/await in our internal code.
On the userland, you can use async/await with your own babel polyfill if necessary, but on Vue Material's internal code, we must think about our bundle size. We can definitely deal with the drawback of nested callbacks.

@FossPrime
Copy link
Contributor Author

FossPrime commented Jul 25, 2018 via email

@Samuell1 Samuell1 merged commit 6fbdd21 into vuematerial:dev Jul 26, 2018
marcosmoura added a commit that referenced this pull request May 11, 2019
* origin: (55 commits)
  fix(MdSvgLoader): svg loader for invalid / missing mimetype (#1942)
  docs(Icon): fix minor typo (#1973)
  fix(MdRouterLink): new router-link registration name (#1978)
  chore: apply discord vanity URL (#1927)
  docs(ROADMAP.md): replace dead slack invite link with discord invite link (#1924)
  docs(Table): fix grammar issues (#1902)
  feat: router link components improvements (#1651)
  fix(MdTable): recalculate fixed header width on data changes (#1877)
  fix(MdDialogContent): missing theme class (#1876)
  docs(Elevation and Layout): Typos fixed and improved documentation. (#1878)
  fix(MdTable): nextTick selection data (#1874)
  docs(BottomBar): fix typo (#1875)
  docs(MdButton): escape a tag (#1872)
  docs(Input): typos and correction
  chore: upgrade dependencies
  fix(MdField): remove firefox required box-shadow
  feat: disable complexity rules for some methods
  fix(MdProgressSpinner): fix CSP error (#1850)
  fix: MdBadge do not accept color #1854 issue (#1856)
  fix: fix the repeated generated css theme classes if there is multiple themes defined (#1784)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants