-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add dynamic data table on collection page #885
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #885 +/- ##
=======================================
Coverage 68.22% 68.22%
=======================================
Files 62 62
Lines 3909 3909
=======================================
Hits 2667 2667
Misses 1242 1242 |
datalab Run #2455
Run Properties:
|
Project |
datalab
|
Branch Review |
ml-evs/add-collection-table
|
Run status |
Passed #2455
|
Run duration | 06m 33s |
Commit |
d699869605 ℹ️: Merge e177fe671432a40cbba1a60c0e0df42bff6f40fd into a28c0b551448f5803317b43e5bfe...
|
Committer | Matthew Evans |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
132
|
View all changes introduced in this branch ↗︎ |
bf89a07
to
95de17d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. Just a few comments or things to modify maybe?
Perhaps we should either remove the checkboxes column inside CollectionInformation, or add a button to delete multiple items from the collection?
And a small eslint warning inside CollectionInformation.vue
because "data" should be above "computed".
95de17d
to
7a6d373
Compare
Thanks Ben, think I've fixed everything
I've removed the checkboxes for now, though we might think about a delete button that removes from just a collection, good idea!
Just raised #895 for this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very great ! 🚀
7259591
to
4d5f3df
Compare
Think this is ready again when you are @BenjaminCharmes! I added a test to catch the case we discussed (also our first for the collections table/page generally). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It look really great, nice one ! 🚀
… for collection edit page
…it for collections
4d5f3df
to
e177fe6
Compare
Thanks, I'm going to merge this now but make a mental note that the tests locally might not work every time |
Closes #815
Closes #891.
Replaces the next table on collections with the new dynamic table, and: