-
Notifications
You must be signed in to change notification settings - Fork 818
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
Spreadsheet module #4313
Spreadsheet module #4313
Conversation
Hey @azmy60 Love it!, im just catching up on things after a couple of months away from the project, but will review your PR in the next week or so. Loving the video though, looks great. Cheers Oli :) |
Hey @olifolkerd ! I have a little follow up commit that'll be coming up tomorrow, and then I'll mark this as ready to review. Also, welcome back! looking forward to your feedbacks! Thanks :) |
I took a quick look, this looks fantastic. @olifolkerd it'd be great to get your feedback on the tabulator-specific bits like event dispatching |
I've been using spreadsheet mode all morning. Overall: It's amazing, and behaves exactly as I'd expect. Great stuff. It even keeps selected ranges when scrolling in a virtual table. Great work on that. Some bugs:
Some missing features that I only notice now I'm using it |
Hey @rathboma thanks for the nudge this one had slipped off my todo, Your notes on user interaction seem spot on, and should be handled through the keybiding module and the internal event bus. I will carve out some time this week to check through each of the events, but from a quick skim i can see a few of the render events could be handled through the built in helper function that come with the module, there are also more optimal ways to refresh the DOM when you are only looking to redraw certain parts, which might be behind some of the performance issues you have noticed. I will have a skim through and put togeather some programatic recommendations, probs by monday eve at this point. If we can work to polish this up i would love to include it in a november release :) Cheers Oli :) |
- if the range is at the edge of the table and the user presses `spreadsheetJump` key, it should stay there instead of moving to the other side of the table. - pressing `spreadsheetJump` should move to the next non-empty cell, just like in google sheet.
fix cannot autoscroll to elements that are not in the DOM
Pressing enter in an input editor does not close it due to the enter key is registered in the keybindings module.
this would add row header each time columns were set
new table options in ResizeColumns: - resizeColumnsHandles - we can put handles on each cells (including headers) or only on the column headers. - resizeColumnsMode - resize the column in realtime or show a guide line when resizing. alongside, we also allow to resize multiple columns at once in when using spreadsheet.
- Clicking a cell would immediately open an editor for that cell. This happens after resizing the column of the cell. - Not able to enter the editor for the first time.
Thanks so much for all your hard work and patients there. Ive just done a patch release to start catching up on some bits while i was away. Im going to look to work through a bit more of the backlog to iron out bits that have been found while i was away and then get this merged into a feature release. Cheers Oli :) |
Hey @azmy60 Thanks for all the continued work on this PR, im putting together the 5.6 release at the moment and would like to include this in that release. I have created a 5.6-spreadsheet branch and merged this PR into it, there are few tweaks im going to need to make to bring this inline with other modules, so i will make them on this branch and test them before it gets merged in. One thing i would note is that this PR has sprawled well beyond the intended spreadsheet functionality, there were white space tweaks, large scale alterations to the column resize module, menu, editor and select row module functionality and a whole new param in the row number formatter. Please keep your PR's restricted to ONE thing, otherwise it becomes impossible to give feedback on items and track changes and approve and merge things in. everything will become held up trying to unpick it all. It is great that you want to make changes, but please do these as separate PR's. I will rollback some of the breaking changes unrelated to the core spreadsheet functionality of this PR and if you wish to reintroduce them, please create separate PR's for each one. I will stick some feedback on here when im done with the tweaks so you can see what was needed for the production release. Cheers Oli :) |
@azmy60 please see Oli's comments above, can you review his branch then create new PRs for the other functionality that we added along the way? @olifolkerd the bundling of features is my fault really as I asked azmy to do it this way to keep the Beekeeper fork simple for us to integrate into the app given how long the branch was open. @olifolkerd do you have a list of the features that you removed so we can make sure we capture each one as an individual PR? |
@rathboma @azmy60 I have made considerable changes to the code from this PR as I have refined it for release and merged these changes into the 5.6 branch so it should now be safe for you to add other changes there as separate PR's. @rathboma The main ones I have removed is the column resize functionality and the rownum formatter pagination updates (the last one needs some work as it doesn't account for grouped row scenarios). I also removed all the menu interactions as these didnt seem to have any affect on range selection at all. Im happy to run you through an changes I've made if you have questions, but the main changes were: Codebase RefactorRefactored the codebase to bring it inline with standard module operating functionality and to bring the code inline with the Tabulator coding style. Ive reordered the functions in the class to group them by function purpose and I have also simplified chunks of the code, optimized parts, and cut down on the duplicated code blocks. There was also a lot of anonymous function usage and a ton of variable hoisting which ive cleaned up for code clarity. Refined Module ScopeAs a spreadsheet module, this code was reaching out all over the table and making a range of very specific changes, a module should do one thing in an isolated fashion and allow for user configuration, so i have rescoped this to a range selection module, if other functionality is needed for your particular usage case it should be included as separate PRs to add new options to modules affected without reference to this module Decouple and Isolate ModuleThis module reached out and directly manipulated several other modules, modules should be decoupled from one another and should communicate via the internal event bus, i have decoupled this module from the core framework and other modules and moved module specific functionality out from other modules and back into this module. Rolled back breaking changesMost of the code added to other modules broke other functionality in those modules, so has been rolled back and moved into this module Class ResponsibilityThe range class was responsible for ranges, but a lot of the code for the ranges sat outside the range class, this has now been moved into the range class, so there is only one class responsible for managing a range. Enhanced OptionsThe module assumed that the user wanted the table to act as a full spreadsheet without considering other usage cases, I have broken the functionality down to the core range selection function and a series of options to give the developer control over the module functionality:
External EventsI've added external events to cover:
Range componentThe range component has been updated to allow programmatic creation and manipulation of a range:
Stylesheet RefactorThe SCSS stylesheet has been refactored to simplify the SCSS variables and to move the styling into the appropriate areas of the stylesheet rather than being tacked on to the end. I have also updated all the themes to style ranges appropriately Console WarningsThis module is not compatible with several configurations of the table, I have added console warnings to help warn a developer when they choose a problematic combination I've merged and pushed my changes into the 5.6 branch. All further PR's will need to be based on the 5.6 branch as the refactor was so significant any changes off the branch of this PR will not be mergeable. Cheers Oli :) |
Hi @olifolkerd , Thanks so much! I'll go through the feedbacks and make the PRs soon! |
I have also just added some cool new range paste functionality that will allow for paste behaviour similar to a spreadsheet, in terms of allowing pasting a value across multiple cells, along with the ability to clear ranges and a few other bits. Hoping to get things release in the next week or so :) Cheers Oli :) |
Version 5.6 has just been released! Including Cell Range Selection Checkout the Release Notes for full details of all the tweaks and features ive added on top of this feature :) Would love to hear your thoughts on it all. Cheers Oli :) |
@olifolkerd Love it! 🥳 Everything is well documented. The improved version of this module is really awesome too! |
Great docs Oli! Looking great. |
Ive just added some tweaks to the 6.0 branch that add some functionality with the range selection module:
I'm working on a few other exciting additions i think you might like over the next few days, will give you a shout when they are in. Cheers Oli :) |
Awesome! |
In attempt to support spreadsheet mode as mentioned in issue #4294
Usage:
Notes:
headerSort
. Clicking the column header would select the cells of the column.