Freeze header row functionality #169

quadrophobiac · 2017-06-26T12:46:54Z

opening a PR to see if I am on the right path with this or in case I'm missing an easy trick.
I have made the minimum changes required to freeze a row for scrolling purposes. I've left it open for a user to freeze multiple rows even though a CSV headers approach would prefer for this not to be the case.
As far as I can tell Electron doesn't support dynamics changes to menu items which would fit with a toggled 'freeze row / unfreeze row' addition to both menu items.
@chris48s I'm blocked on the inclusion of these functions in the main process menu. Basically it seems like nothing is firing between main and renderer. Was wondering if you could take a glance at the code and tell me if I am doing anything glaringly wrong or if there is a better way of writing code the communicates between the main and renderer process in Electron nowadays

Fail first

multiple rows may be frozen using this method. Function to unfreeze rows also required

Using adequate JS object accessors

This will reset hands on table fixed Row variable and remove any frozen rows from the UI

Uncertain what the issue is here but messages not being sent to enable user to use Main process menu for freezing/unfreezing rows

quadrophobiac · 2017-06-26T12:52:42Z

for fix to #148 sans the header-dialect functionality

Stephen-Gates · 2017-06-26T13:01:40Z

Should be able to toggle menu value. Implemented in Atom

chris48s · 2017-06-26T19:18:52Z

Really quick off-the-cuff response. I've not checked out the code and run it, but I think the piece of the jigsaw you are missing is that you haven't defined handlers for the freezeRows and unfreeze events.

If you add a few lines to https://github.com/theodi/comma-chameleon/blob/master/renderer/index.js declaring handlers like

ipc.on('freezeRows', function() {
  // do the stuff
});

..that should get you moving :)

quadrophobiac · 2017-06-27T11:24:33Z

Thank you for the pointer @chris48s , still reacclimatising to Electron environment, that's unblocked it!

quadrophobiac · 2017-06-27T11:29:24Z

This is basically the bare bones functionality to freeze the header. What could be improved in subsequent patches are...

colour coding of rows frozen
consolidation of freeze and unfreeze functions into a single function that toggle in the relevant Electron menus
CSV Dialect Support indicated in Freeze header functionality #148

Additions suffice to add mvp user functionality

chris48s · 2017-06-27T21:30:50Z

This seems to behave differently depending on whether you drag a selection down from the top or up from the bottom. For example, here I've dragged the selection down from the top:

I'd expect that to freeze the top 4 rows, but it does nothing. I see that is because the 'selected' cell is in the top row, but I think this is still a bit counter-intuitive (is this just me that expects this behaviour?). Would an easier way to make this clearer be to only work with a single row selection? If others think the behaviour I'm expecting here is strange, feel free to ignore this point.

Also sometimes it freezes just the row numbers, not the whole row, like this:

.. but I can't quite work out the steps to reproduce that. I would expect that to do this:

Also a few more minor observations:

If you click unfreeze on the right-click menu, you get 'stuck' in a select. I think unfreeze() should call hot.deselectCell(); at the end of the function to prevent this.
The operations in the Edit menu and right-click menu are called 'Freeze Rows' and 'Freeze row(s) above' which makes it sound like they do might do slightly different things but I think they do the exact same thing.
Can the events be called freezeRows and unfreezeRows or freeze and unfreeze for consistency.

Rather than enabling user to freeze multiple rows this change stipulates that only the header row may be frozen. I reason this is consistent with Comma Chameleon as constrained editor design principle (in addition to being more straightforward). Calling updateSettings() twice is a hack but is required due to how the render callbacks execute. The second call could be used to set `colHeaders` to false, enabling the user to use their header row as GUI headers

quadrophobiac · 2017-07-13T14:07:25Z

@chris48s I made the suggestions, with more detail here.
To get the row to freeze consistently I had to kludge it though - calling updateSettings twice. I'm not sure if that's necessary or if my understanding of javascript is leading to a very poor solution. However when I tested the callbacks [via script from here https://docs.handsontable.com/0.16.1/tutorial-callbacks.html] I could see that the re-rendering was executing prior to the fixedRowsTop value being altered. I'm inclined to think that this might be a bug that could be fixed in subsequent versions of HandsOnTable but even still I'm not certain about incorporating the kuldge to codebase - thoughts?

chris48s · 2017-07-13T14:46:39Z

I've got several bits of time-consuming life admin going on at the moment so I'm going to aim to look at this on Monday evening next week for you. If that's blocking you, happy to defer to @pezholio or @Floppy 's reckons :)

chris48s · 2017-07-16T18:55:52Z

renderer/hot.js

+      },
+      afterLoadData: function() {
+          loader.hideLoader();
+      },


Add a handler for the afterUpdateSettings event here

chris48s · 2017-07-16T18:56:31Z

renderer/index.js

@@ -153,3 +153,10 @@ ipc.on('removeRows', function() {
 ipc.on('removeColumns', function() {
  hotController.removeColumns();
 });
+
+ipc.on('freezeRows', function(){


This event hander should be updated to expect an event called 'freeze' to match with the event you're raising here

chris48s · 2017-07-16T18:56:52Z

renderer/menu.js

+});
+
+var unfreezeRow = new MenuItem({
+    label: 'unfreeze header row',


'Unfreeze Header Row'

chris48s · 2017-07-16T18:58:24Z

Managed to slot in a bit of time to look at this tonight for you. All the odd behaviours I reported in my previous post seem to be fixed now, so that's useful.

There's one minor problem: The right-click menu item works but the 'Freeze Header Row' in the Edit menu item doesn't. It is firing an event called freeze but the registered handler is expecting an event called freezeRows. I've left a line note where it needs to be changed.

To get the row to freeze consistently I had to kludge it though - calling updateSettings twice. I'm not sure if that's necessary or if my understanding of javascript is leading to a very poor solution

Yeah - I can see the issue you were having. I'm also not entirely sure if this is intended behaviour for HoT or a bug we need to work around, but I think there is a neater way to ensure the re-rendering occurs after the setting are applied. Define a afterUpdateSettings handler when we pass the initial config to the Handsontable constructor, like this:

...
afterUpdateSettings: function() {
  hot.render();
  hot.deselectCell();
},
...

I've also added a line note where that declaration should live. Then that allows you to simplify your event handlers to:

var unfreezeHeaderRow = function(){
    hot.updateSettings({fixedRowsTop: 0, colHeaders: true});
};

var freezeHeaderRow = function(){
    hot.updateSettings({fixedRowsTop: 1}, false);
};

and deal with the cleanup in the handler function (which is guaranteed to execute after the settings have been applied). Maybe also note in issue #159 that we might be able to remove that manual call to render() when we upgrade the library.

quadrophobiac · 2017-07-17T10:55:09Z

Hey @chris48s , thanks so much for taking the time to give me solid js pointers on how to achieve the requisite functionality in a more cohesive way really appreciate the tuition you're imparting! And aside from personal gratitude I'd also like to thank you because having your insight on hand really helps the work that can be done on Comma Chameleon during present circumstances - so thanks from the Toolbox too :)

chris48s · 2017-07-17T11:44:00Z

no probs - glad to help :)

quadrophobiac · 2017-07-17T12:18:26Z

Cool, so I'm taking your input as a code review @chris48s and I'm going to merge this in now

quadrophobiac added 10 commits June 1, 2017 17:47

Test for freezing one row

55335e9

Fail first

Function to freeze rows

2ce46f7

multiple rows may be frozen using this method. Function to unfreeze rows also required

Include freeze rows in csv editor submenu

6f83539

Passing test for addition

c944813

Make test terser

f8c8efa

Using adequate JS object accessors

Include freeze in main menu

6b80474

Merge branch 'master' into freeze-header-row-functionality

c79f7f1

Include an unfreeze function

39b56e3

This will reset hands on table fixed Row variable and remove any frozen rows from the UI

Include freeze / unfreeze in right click menu

fa0eb03

Blocked - messaging from main to renderer fail

840a9d3

Uncertain what the issue is here but messages not being sent to enable user to use Main process menu for freezing/unfreezing rows

Addition of IPC calls

ee08468

Add semicolons to please jshint

d5bed83

quadrophobiac requested review from Floppy and pezholio June 27, 2017 13:58

quadrophobiac added fn:Usability i:enhancement labels Jul 6, 2017

quadrophobiac added 4 commits July 12, 2017 17:01

Merge branch 'master' into freeze-header-row-functionality

635178e

Rename menu function calls to be consistent

74af6f9

Update tests to reflect refactored functions

f7195d9

quadrophobiac added the f:help-wanted label Jul 13, 2017

chris48s reviewed Jul 16, 2017

View reviewed changes

quadrophobiac added 5 commits July 17, 2017 11:35

establish event handler to guarantee render with new settings

fb6ecad

Correctly name event in IPC messaging

25bbf97

Consistent capitalisation for menu

b5e787b

Make freeze functions terse again

9d96494

Correct name for IPC messaging call

3d3194c

quadrophobiac mentioned this pull request Jul 17, 2017

Upgrade HandsOnTable Library #159

Open

quadrophobiac merged commit 7d0d9ec into master Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Freeze header row functionality #169

Freeze header row functionality #169

quadrophobiac commented Jun 26, 2017 •

edited

Loading

quadrophobiac commented Jun 26, 2017

Stephen-Gates commented Jun 26, 2017

chris48s commented Jun 26, 2017

quadrophobiac commented Jun 27, 2017

quadrophobiac commented Jun 27, 2017

chris48s commented Jun 27, 2017

quadrophobiac commented Jul 13, 2017

chris48s commented Jul 13, 2017

chris48s Jul 16, 2017

chris48s Jul 16, 2017

chris48s Jul 16, 2017

chris48s commented Jul 16, 2017

quadrophobiac commented Jul 17, 2017 •

edited

Loading

chris48s commented Jul 17, 2017

quadrophobiac commented Jul 17, 2017

Freeze header row functionality #169

Freeze header row functionality #169

Conversation

quadrophobiac commented Jun 26, 2017 • edited Loading

quadrophobiac commented Jun 26, 2017

Stephen-Gates commented Jun 26, 2017

chris48s commented Jun 26, 2017

quadrophobiac commented Jun 27, 2017

quadrophobiac commented Jun 27, 2017

chris48s commented Jun 27, 2017

quadrophobiac commented Jul 13, 2017

chris48s commented Jul 13, 2017

chris48s Jul 16, 2017

Choose a reason for hiding this comment

chris48s Jul 16, 2017

Choose a reason for hiding this comment

chris48s Jul 16, 2017

Choose a reason for hiding this comment

chris48s commented Jul 16, 2017

quadrophobiac commented Jul 17, 2017 • edited Loading

chris48s commented Jul 17, 2017

quadrophobiac commented Jul 17, 2017

quadrophobiac commented Jun 26, 2017 •

edited

Loading

quadrophobiac commented Jul 17, 2017 •

edited

Loading