-
Notifications
You must be signed in to change notification settings - Fork 149
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
Resize column widget on PaginatedDataTable2 #93
base: main
Are you sure you want to change the base?
Conversation
PaginatedDataTable2
- Changed icons on Draggable widget - Now the columns move with the mouse pointer
Thanks for submitting the PR, the feature seems great. Though there're a few issues:
JIC, here's my flutter doctor -v output [✓] Flutter (Channel stable, 3.0.1, on macOS 12.3.1 21E258 darwin-x64, locale en-GB) [✓] Android toolchain - develop for Android devices (Android SDK version 31.0.0) [✓] Xcode - develop for iOS and macOS (Xcode 13.4) [✓] Chrome - develop for the web [✓] Android Studio (version 2020.3) [✓] VS Code (version 1.67.2) [✓] Connected device (2 available) [✓] HTTP Host Availability |
Fixed the failing unit tests and the exception when resizing the window. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
==========================================
- Coverage 95.87% 88.97% -6.91%
==========================================
Files 3 4 +1
Lines 1140 1197 +57
==========================================
- Hits 1093 1065 -28
- Misses 47 132 +85 ☔ View full report in Codecov by Sentry. |
By default DataTable2 expands to it's container, it's width is determined by the number of pixel LayoutBuilder provides. I.e. the default scenario is that DataTable2 doesn't have a fixed width, should anyone want a fixed width, the control must be wrapped in limmiting widget. For the cases when viewport becomes to narrow and Columns get condensed to non-reasonable widths there's a Speaking of resizing columns expected behaviour:
Here're some thoughts which I'm not quite sure yet in terms of implementability/consictency, your considerations are welcome:
|
One more thing, DataTable2 is stateless while PaginatedDataTable2 is statefull, apparently dragging columns will require to adress that issue cause it might be to reasonable to have that behavior implemented in both widget. Yet making DataTable2 stateful might be a poor option cause it is treated as direct replacement to Flutter's DataTable which is statless - not usre if switching DataTable2 to staefull might present any challenges in this regard |
I don't see any problem resizing the total width of the table when resizing a column, it's not uncommon. In other platforms that implement resizing columns it works this way, and I think if you want resizable columns you should expect variable table width. And if, for example, all the columns are defined with fixed width, I don't see another way other than resizing the table. Maybe if the columns are defined with proportional widths, we could shrink the columns first to its minimum before growing the table.
I think this could be a bit cumbersome, specially for large tables with many columns, you would have to scroll to the rightmost column in order to give more space for other columns. It would be much easier if the table grows/shrink with the column.
I agree with this, we could design a draggable widget that is both compatible with desktop and mobile, or give an option to select which mode you want to use (desktop / mobile) or put some parameters to customize the looks. I'll see what I can do.
Yeah, this is a tricky subject, I didn't implement column resizing on DataTable2 to keep it stateless. Maybe we should clarify in the code comments that the function Another thing I found, I tested this feature on another computer on Linux and I found the lag / misalignment problem a bit worse, I'll see if I can find a better approach for the resizing. |
- Added validation of minimum column size - Added controller for data columns - Now columns custom widths are kept when sorting columns or repainting the table - Now non fixed width columns are resized first, if minimum is reached, the total table width is increased - When a column is resized, is treated as fixed width - During resizing, the column size now follows the mouse pointer - Removed resizable column in DataTable2 example, as is not intended to be used here - Added resizable columns to route options of PaginatedDataTable2 example
I made some progress on the pending tasks. These are the major changes:
I'm wondering if it would be best to create a new stateful DataTable class to implement the resizing there and keep all the implementation in a data table. Let me know your thoughts. |
@nerdknight - thanks! The sample looks great, I like the way you styled the draggable. Here're the improvements to put into the backlog in regards to it's looks:
In terms on stateless/stateful I think there's no other better option rather than create a new widget StatefulDataTable2 and put the resizing logic there (encapsulating/aggregating DataTable2). There's also Speaking of DataTable2, it's necessary to make as few changes as possible to it's internals and move most of the logic to StatefulDataTable2 - keep the class Open for extensions but Closed for modifications - at least as much as possible. Besides DataTable2 and thus DataColumn2 should not leak any of the stateful features, such as resizing, there shouldn't be any public APIs in that regards - that's why I've introduced ResizableDataColumn2 inherited from DataColumn2. I've committed some drafts of the above mentioned refactoring. Let me know what you think. |
Speaking of column resizing, please see below an interesting bug. Please not that at the beginning of the video resizing columns leads to flexing all rows around. Yet when you change width of the Name column all other columns to the right get resized while column to the left are untouched - the second part IMO is the right behaviour: resizing.bug.movHere's how I see resizing behaviour:
Also a separate example (1st level route) is required for the resizing scenarious with 2 subroute - one depicting PaginatedDataTable2 and another StatefulDataTable2, that will make examples easer to understand |
The refactor you made looks good, I'll start working with that and the other proposed changes.
The actual behavior is a bit awkward, but it was intended this way: all the columns with proportional size (
It could be implemented, but with the actual behavior, if |
IMO, fixedWidth was not the best choice of naming, something like absoluteWidth would be more relevant. It's just an alternative way of defining default or standard sizing behavior for columns - you either define pixels and assume column doesn't respond to table width changes or provide relative size via S, M or L. None of the options must force the resizing logic to ignore any of the 2 kinds of rows. After all we have the resizable column type with a separate flag devoted to controlling locking or unlocking resizing, no need to add another parameter (checking if column is fixedWidth size). Though adding a documentation remark of stateful table regarding fixed column sizes might be useful. Speaking of implementation detail, I think that looking into using fixed width columns to achieve column resizing upstream with zero to None changes to DataTable2 might be a good option. I.e in the StatefulDataTable2 on resizing thr build() method can instantiate a new table supplying it with clones of provided columns BUT having their fixedWidth set to specific values. Note, a method on ColumnSizeControl controller can be added to reset column width adjustments. |
- Implemented StatefulDataTable2 - Added new example for StatefulDataTable2 - Now PaginatedDataTable2 uses StatefulDataTable2 instead of DataTable2
Checked the recent commit, assuming it is still a WiP I won't be treating it as the one requiring much of feedback. Though I'm also curious if you have investigated rebuilding DataTable2 inside StatefulDataTable2 and recreating columns with fixed width upon resizing. |
Thanks for tidying up the examples and presenting a new one! |
Thanks for your feedback maxim. I haven't had much time to work on this lately, an yes the refactor is still work in progress, if you like I could do partial commits so you can see the progress and give some feedback. Something similar may be done with Another question I have, what do we do if we have a StatefulDataTable2 with a mix of fixed and non-fixed width columns and the window resizes? The actual behavior of DataTable2 is to resize only non-fixed-width columns. If we treat all columns as fixed, do we resize all columns proportional regardless of their initial settings? or resize only the rightmost column? and if some columns are not resizable? should they keep their initial settings?. I implemented the current logic to maintain this behavior of DataTable2. Rebuilding DataTable2 was one of the approaches I tried earlier but I think I discarded it for this reason. |
IMO, if a column is resized it keeps that size no matter if the width of parent is changed. Should the parent grow and all columns have their widths altered, they should stay that way. The only column changing it's width should be the rightmost one. Optionally there can be a special mode which can adjust manually altered columns proportionally to parent change - though that mode is not trivial to describe (e.g. what if there's already a horizontal scroll bar and table total width is greater than viewport) |
Hi @nerdknight , should you still be looking at the PR... I think I've come up with a more elegant solution to having state which allows to keep DataTable2 stalest and avoid creating StatefulDataTable2. You can use a stateful widget inside and builder pattern to feed column width's inside. You can check the recent commit and |
Now the non fixed size columns to the left the one beeing resized don't change their size
Hey there. It is taking me time to review the changes. I'm going slowly as there's a lot of new code and not trivial one. Started with refactoring attempting to make the changes easier to understand. I'm still not very happy with the behaviour of resizing of a column and having leftmost columns changing sizes as well, that seems a bit odd to me. Though I am still not well aquainted with the code to be specific of implementation suggestions. |
Looks like you've changed that behaviour in the recent commits. Thanks! |
Hi Maxim, yes the previous behavior was a bit odd. Now columns to the left don't change size, the columns to the right resize if they are defined with proportional size. Let me know if you have any suggestions. |
@maxim-saplin Could you close this branch ? |
@maxim-saplin is it done ? |
It is not |
…iddle of column line separator
Widget to resize columns manually. It can be anabled by setting to true the
resizeable
variable in theDataColumn2
constructor.https://user-images.githubusercontent.com/5269947/169674825-17f5f538-7c22-417e-8b41-297605912657.mov