PDA

View Full Version : [FIXED] several issues with Grid component



geniny
16 May 2017, 12:10 PM
Hi,

I started evaluating the ExtReact and found several issues with the Grid component. I understand it is a fresh release and bugs are expected but wanted to give you heads-up on what I found so far:

1. Grouping does not work at all. I used the bootstrap repo and choosing 'Group by this field' menu has no effect. Also, in the Grouping example it works but you cannot ungroup a column once it is grouped. Choosing 'Show in groups' does not do anything (it is checked when clicked but has no effect).

2. Sorting by column behaves erratically. If you click on 'Sort Ascending' Once it works. But then clicking on the same menu multiple times just removes the Up arrow and never puts it back. Even clicking on 'Sort Ascending' on a different column after that has no effect. It only resets it if you click 'Sort Descending' on any column menu.

Thank you,
Yuri

Mark.Brocato
16 May 2017, 1:30 PM
Hi Yuri,

Thank you for the feedback. We're working on the next update, v6.5.1 right now. We'll incorporate this feedback in our backlog.

geniny
16 May 2017, 1:45 PM
Other grouping issues:

1. Setting groupable to false on a column has no effect. Grouping menus are not hidden. 'Group by this field' is not disabled and selecting it groups the grid by that field.
2. Setting grouped to false on the grid itself does not remove grouping menus from columns and 'Group by this field' menu is still enabled.

geniny
17 May 2017, 3:28 PM
I found why the 'Show in groups' menu click does not remove the column from grouping. Code is in the Column.js:


onToggleShowInGroups: function (menuItem) {
if (!menuItem.getChecked()) {
var grid = this.getGrid(),
store = grid.getStore();

store.setGrouper(null);
}
},


The highlighted line should be:

if (menuItem.getChecked()) {

because when the column is grouped the checkbox is checked.

Please fix it!

Mark.Brocato
17 May 2017, 4:14 PM
Thanks for the report! I have opened a bug in our bug tracker.

Mark.Brocato
17 May 2017, 5:16 PM
Yep, that seems to fix it alright. Until 6.5.1 comes out, you can apply that fix to your app in an override, just like you would with Ext JS. If you're using the boilerplate, you can add a file called to ext-react/overrides (the name of the file doesn't matter) with the following code:



Ext.define('ExtReact.override.ColumnGroupFix', {
override: 'Ext.grid.column.Column',

onToggleShowInGroups: function (menuItem) {
if (menuItem.getChecked()) {
var grid = this.getGrid(),
store = grid.getStore();


store.setGrouper(null);
}
}
});


The location of overrides is configured via the ExtReactWebpackPlugin:
http://docs.sencha.com/extreact/6.5.0/guides/webpack_and_babel.html#webpack_and_babel_-_overrides

geniny
18 May 2017, 7:19 AM
I was able to identify the bugs related to sorting too (Grid modern toolkit, ExtReact)

Problem 1. In any column menu select 'Sort ASC'. Up arrow appears in the column. Select 'Sort ASC' again. The up arrow disappears. Selecting 'Sort ASC' again any number of times will not result in sorting and will be ignored by the grid.

Problem 2. Click on the column header 1. Column 1 is sorted ASC. Click on the column header 2. Column 2 is sorted ASC and column 1 is not sorted. Click on the column header 1 again. Column 1 is sorted DESC which is wrong (it should not remember what was the previous sort order and in your Ext classic toolkit grid it works as expected).

Here's where you have to make changes in your code to fix both of these (node_modules/@extjs/ext-react-toolkit/src/grid/column/Column.js):



onSortDirectionToggle: function (menuItem) {
var me = this,
grid = me.getGrid(),
store = grid.getStore(),
sorter = me.getSorter(),
sorters = store.getSorters(),
isSorted = sorter && (sorters.contains(sorter) || sorter === store.getGrouper()),
direction = menuItem.direction;

// Remove sorter on uncheck if its the matching direction
if (isSorted && sorter && sorter.getDirection() === direction) {
sorters.remove(sorter);

// Store will not refresh in response to having a sorter removed, so we must
// clear the column header arrow now.
me.setSortState(null);
}
else {
// If have no sorter, or store is not sorting by that sorter, or the sorter
// is opposite to what we just checked then sort according to the CheckItems's
// direction
if (!isSorted || sorter.getDirection() !== direction) {
me.sort(direction);
}
}
}






sort: function (direction) {
var me = this,
sorter = me.getSorter(),
grid = me.getGrid(),
store = grid.getStore(),
isSorted = sorter && store.getSorters().contains(sorter);

// This is the "group by" column - we have to set the grouper and tellit to recacculate.
// AbstractStore#group just calls its Collection's updateGrouper if passed a Grouper
// because *something* in the grouper might have changed, but the config system would
// reject that as not a change.
if (store.isGrouped() && store.getGroupField() === me.getDataIndex()) {
sorter = store.getGrouper();
me.setSorter(sorter);
if (sorter.getDirection() !== direction) {
sorter.toggle();
store.group(sorter);
}
return;
}

if (isSorted && sorter) {
// Our sorter is in the requested direction
if (sorter.getDirection() === direction) {
// If it is applied, we've nothing to do
if (isSorted) {
return;
}
} else {
me.oldDirection = sorter.getDirection();
sorter.toggle();
}
} else {
me.setSorter({
property: me.getSortParam(),
direction: direction
});
sorter = me.getSorter();
}

// If the sorter is already applied, just command the store to sort with no params.
// If the grid is NOT configured with multi column sorting, then specify "replace".
// Only if we are doing multi column sorting do we insert it as one of a multi set.
store.sort.apply(store, isSorted ? [] : [sorter, grid.getMultiColumnSort() ? 'multi' : 'replace']);
},



Please let me know if this makes sense.

Thank you,
Yuri

geniny
19 May 2017, 7:01 AM
Hi,

Any response to the last post?

The fix in both sections of code is in bold.

Thank you,
Yuri

Animal
19 May 2017, 7:20 AM
Hi geniny, thanks for bringing this to our attention, and thanks for digging in a bit!

We will certainly fix this in our next maintenance release.

Animal
21 May 2017, 12:08 PM
OK, this is what we are proposing. Three states. ASC, DESC and no sorter on that data field.

The header cycles through in that order on clicks. The menu items are mutually exclusive checkboxes. Both sorting UIs will stay in sync

It works like this:

https://s3.amazonaws.com/uploads.hipchat.com/9804/25186/gWMNCPZbINilVCn/checkable-sortmenu.gif

geniny
21 May 2017, 12:50 PM
From the usability perspective, it is better to have three exclusive menus (Sort ASC, Sort DESC, No Sort) and use Radio not Check images to depict it.

Thanks,
Yuri

geniny
22 May 2017, 6:54 AM
I would suggest to have three menu items instead: Sort ASC, Sort DESC, and No Sort. They also need to behave as radio not as a checkbox since they are three mutually exclusive states. So the radio button image would be better reflecting the behavior.

Thank you,
Yuri

Animal
22 May 2017, 11:04 PM
I forgot to demo that part. Those two are in fact Ext.menu.RadioItem (http://docs.sencha.com/extjs/6.5.0/modern/Ext.menu.RadioItem.html)s in their own group (http://docs.sencha.com/extjs/6.5.0/modern/Ext.menu.RadioItem.html#cfg-group), so they are mutually exclusive. Only one may be checked at a time.

The tick image used in RadioItems may be themed using SASS variables (http://docs.sencha.com/extjs/6.5.0/modern/Ext.menu.RadioItem.html#css_var-S-$menuradioitem-checkbox-icon) to use another glyph.

I think adding a third option would muddy the design and add possible confusion over a redundant option. The items are uncheckable (http://docs.sencha.com/extjs/6.5.0/modern/Ext.menu.RadioItem.html#cfg-allowUncheck), so you simply uncheck the item to remove the Sorter.

geniny
23 May 2017, 7:46 AM
When you remove sort from a column does it issue a fresh request to the server through a proxy (remote sorting case)?

Animal
23 May 2017, 1:03 PM
No. It doesn't right now. It simply removes the Sorter (http://docs.sencha.com/extjs/6.5.0/modern/Ext.util.Sorter.html) owned by that column from the Store (http://docs.sencha.com/extjs/6.5.0/modern/Ext.data.Store.html).

The assumption being that you cannot "unsort" data. I can see that possibly not being true if the server has its own ideas about order when not being commanded to utilize a certain sort.

Not sure if reloading on removal of a remote mode sorter should *always* reload the dataset, or if it should be configurable behaviour. I'll discuss it with the guys here.

geniny
23 May 2017, 1:39 PM
Yes, agree, it can be configurable.

Animal
24 May 2017, 6:20 AM
OK, I propose this new config option. Does this sound about right?



/**
* @cfg {Boolean} [reloadOnSorterRemove=false]
* Only valid when {@link #cfg!remoteSort} is `true`.
*
* By default, the store reloads itself when a sorter is added.
*
* When a sorter is removed, however, the assumption is that the data cannot be "unsorted",
* and no reload is triggered.
*
* If the server has a default order which it reverts to in the absence of a sorter, then
* set this config to `true`, and any change in the number of sorters will trigger a reload.
*/
reloadOnSorterRemove: null

geniny
24 May 2017, 6:32 AM
Yes, exactly what we need. But why the default is null, not false?

Mark.Brocato
16 Jun 2017, 11:14 AM
The latest prerelease of @extjs/reactor allows you to render React elements in grid cells. To do so, add the following to your imports:


import { RendererCell } from '@extjs/ext-react';

and then use the renderer prop on Column like so:



<Column dataIndex="someField" renderer={(value, record) => <MyComponent value={value} record={record}/>} />