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

refactor: data-table #5737

Merged
merged 32 commits into from
Feb 22, 2019
Merged

refactor: data-table #5737

merged 32 commits into from
Feb 22, 2019

Conversation

nekosaur
Copy link
Member

@nekosaur nekosaur commented Nov 26, 2018

Description

Another rewrite of data-table. This time still using html table elements.

Implementing new functionality such as multi-column sorting, group by column, sort by column, per-column filtering, and fixed headers. Fixed headers will not be available in IE11 as I'm using fixed css property.

Virtualization is being done by using dummy rows prepended and appended to tbody that expand in height to simulate the full list of items.

The following issues (and more) have been implemented so far
#1547, #2659, #2859, #3102, #3176, #3180, #3574, #3486, #2960, #3829, #3220, #2890, #3364, #6138

Motivation and Context

How Has This Been Tested?

Markup:

https://gist.github.com/nekosaur/8ffc4996d033a71d9dbff9060eb6765c

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes, dev for new features and breaking changes).
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have created a PR in the documentation with the necessary changes.

@nekosaur nekosaur added this to the v2.0.0 milestone Nov 26, 2018
@nekosaur nekosaur mentioned this pull request Nov 26, 2018
9 tasks
Copy link
Member

@johnleider johnleider left a comment

Choose a reason for hiding this comment

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

Did a light run through. I think this is a good opportunity to discuss the different types of features/functionality that can come with a component and where do they go. Check https://github.com/vuetifyjs/vuetify-next for next proposed structure @vuetifyjs/core-team.

},

methods: {
toggle (key: string, oldBy: string[], oldDesc: boolean[], page: number, mustSort: boolean, multiSort: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

separate lines for this many args

packages/vuetify/src/components/VData/VDataFooter.ts Outdated Show resolved Hide resolved
children.push(regularSlot)
} else {
if (props.mobile) {
children.push(h('div', { class: 'd-flex justify-content-between' }, [
Copy link
Member

Choose a reason for hiding this comment

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

These are helper classes and will make it hard to overwrite in userland

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I know. A lot of this is still work in progress and/or placeholdery

@dsseng dsseng added the S: has merge conflicts The pending Pull Request has merge conflicts label Dec 31, 2018
@codecov
Copy link

codecov bot commented Jan 1, 2019

Codecov Report

Merging #5737 into dev will decrease coverage by 6.81%.
The diff coverage is 49.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #5737      +/-   ##
==========================================
- Coverage   88.56%   81.74%   -6.82%     
==========================================
  Files         282      276       -6     
  Lines        6313     6438     +125     
  Branches     1586     1594       +8     
==========================================
- Hits         5591     5263     -328     
- Misses        604      955     +351     
- Partials      118      220     +102
Impacted Files Coverage Δ
packages/vuetify/src/locale/en.ts 100% <ø> (ø) ⬆️
...ages/vuetify/src/components/VDataIterator/index.ts 100% <ø> (ø)
...es/vuetify/src/components/VDataTable/VTableBody.ts 0% <ø> (ø)
packages/vuetify/src/components/VCheckbox/index.js 100% <100%> (ø) ⬆️
...ackages/vuetify/src/components/VDataTable/index.ts 100% <100%> (ø)
packages/vuetify/src/components/index.ts 100% <100%> (ø) ⬆️
packages/vuetify/src/components/VData/index.ts 100% <100%> (ø)
...ify/src/components/VDataTable/VDataTableVirtual.ts 11.53% <11.53%> (ø)
...ges/vuetify/src/components/VDataTable/VRowGroup.ts 13.33% <13.33%> (ø)
...rc/components/VDataTable/VDataTableHeaderMobile.ts 17.24% <17.24%> (ø)
... and 137 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a95e2ec...c1f009e. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 1, 2019

Codecov Report

Merging #5737 into next will decrease coverage by 5.77%.
The diff coverage is 12.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #5737      +/-   ##
==========================================
- Coverage   89.03%   83.26%   -5.78%     
==========================================
  Files         310      316       +6     
  Lines        7561     7844     +283     
  Branches     1869     1936      +67     
==========================================
- Hits         6732     6531     -201     
- Misses        729     1209     +480     
- Partials      100      104       +4
Impacted Files Coverage Δ
packages/vuetify/src/locale/en.ts 100% <ø> (ø) ⬆️
...ages/vuetify/src/components/VDataIterator/index.ts 100% <ø> (ø)
...c/components/VDataTable/VDataTableHeaderDesktop.ts 10.25% <10.25%> (ø)
packages/vuetify/src/components/VData/index.ts 100% <100%> (ø)
...ackages/vuetify/src/components/VDataTable/index.ts 100% <100%> (ø)
packages/vuetify/src/components/index.ts 100% <100%> (ø) ⬆️
packages/vuetify/src/components/VCheckbox/index.js 100% <100%> (ø) ⬆️
...ify/src/components/VDataTable/VDataTableVirtual.ts 11.11% <11.11%> (ø)
packages/vuetify/src/util/helpers.ts 72.25% <12.19%> (-20.17%) ⬇️
...ges/vuetify/src/components/VDataTable/VRowGroup.ts 13.33% <13.33%> (ø)
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0ebde1...3fb1704. Read the comment docs.

Copy link
Contributor

@dsseng dsseng left a comment

Choose a reason for hiding this comment

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

@nekosaur please resolve merge conflicts

@dsseng dsseng added T: enhancement Functionality that enhances existing features and removed S: has merge conflicts The pending Pull Request has merge conflicts labels Jan 2, 2019
@nekosaur nekosaur force-pushed the refactor/data-table-mono branch from 7d69acc to d449e27 Compare January 16, 2019 22:38
@nekosaur nekosaur changed the base branch from dev to next January 16, 2019 22:39
@grishnyakov
Copy link

This may be useful: multicolumn sort
https://codepen.io/grishnyakov/pen/mawOXg

@nekosaur
Copy link
Member Author

multi-column sort has been implemented already

@nekosaur nekosaur force-pushed the refactor/data-table-mono branch from d449e27 to 28db79e Compare January 18, 2019 21:08
@johnleider
Copy link
Member

Some feedback:

  • Show page number
    This should be in the center
    image

  • Custom header/footer with provide
    Had multiple console errors
    image

  • Custom expanded example
    Expand all doesn't work

  • Filter by column
    Had multiple console errors
    image

I also was not able to figure out how to get resizable columns to work.

Other than that, everything is looking really good.

@nekosaur
Copy link
Member Author

nekosaur commented Feb 4, 2019

Ah, I might not have uploaded the latest playground.

dsseng
dsseng previously requested changes Feb 4, 2019
Copy link
Contributor

@dsseng dsseng left a comment

Choose a reason for hiding this comment

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

Please resolve merge conflicts.

@nekosaur
Copy link
Member Author

nekosaur commented Feb 4, 2019

@sh7dm This is far from done, no need to ask for merging.

@vercel vercel bot temporarily deployed to staging February 9, 2019 16:50 Inactive
@nekosaur nekosaur force-pushed the refactor/data-table-mono branch from d4d5ec1 to 39a2d9d Compare February 9, 2019 16:54
@TravisBuddy
Copy link

TravisBuddy commented Feb 9, 2019

Hey @nekosaur,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 1e02e110-368b-11e9-afc2-91cde96227b3

@johnleider
Copy link
Member

I attempted to resolve merge conflicts but there were quite a few broken tests, are you tracking this @nekosaur ?

@prune998
Copy link

Please keep on @nekosaur !! We all need this feature !!

added pageCount event
added progress slot
added top slot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: has merge conflicts The pending Pull Request has merge conflicts S: work in progress T: enhancement Functionality that enhances existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants