Skip to content
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

SS4 BUG : GridFieldSortableHeader - sort heading disappears when clicked #7610

Closed
1 task done
SpiritLevel opened this issue Nov 17, 2017 · 3 comments
Closed
1 task done

Comments

@SpiritLevel
Copy link
Contributor

SpiritLevel commented Nov 17, 2017

Before clicking on any sort heading:

screenshot from 2017-11-17 14-13-41

After clicking on TITLE, the heading CHANGED disappears:
screenshot from 2017-11-17 14-14-20

After refreshing then clicking on CHANGED, the heading CHANGED disappears:
screenshot from 2017-11-17 14-14-01

Also, layout for GridFieldFilterHeader hides the 'X' for closing it:
screenshot from 2017-11-17 14-20-53
And, the general formatting leaves a lot of white space while the CHANGED dates are forced to wrap to several lines. :)

Pull Requests

@unclecheese
Copy link

Researched this with @phalkunz and found the cause of the issue. Solution could be done multiple ways, but they all feel like hacks. Needs more discussion.

Cause of issue

On clicking a filterable header, the ajax handler re-renders the grid, and checks to see if filters are on. If they're not, it injects the search icon button on the last cell of the header. This indiscriminately replaces whatever content is in there. In this case, because there are no row actions defined, that last cell contains content.

A secondary, perhaps more consequential, problem, is that the grid doesn't render with the filter trigger button on initial load, which means, unless you have edit/delete actions defined, you can't filter the grid, even though you have a filter header component on it.

How to fix

We need to ensure the grid loads with a search trigger appended to the header row. This affects four components:

  • GridFieldSortableHeader
  • GridFieldFilterHeader
  • GridFieldDataColumns
  • GridFieldToolbarHeader

Currently, there is an implicit dependency created between SortableHeader and FilterHeader, as the sortable header checks to see if the gridfield has a filter installed, and if so, it injects a search button. However, the logic is set up, somewhat appropriately, to ensure that the header cells are equal to the data column cells.

We basically need to add a new cell to the sort header if a filter is installed, and also an empty column to the data columns to accomodate it. We tried making GridFieldFilterHeader a column provider, but it simply appends the empty column to the front of the row, which is not what we need. There doesn't seem to be an API for specifying what position you want to add the new cell.

Alternatively, since we already have JS that is injecting button HTML from a component, maybe we should just hack the grid client side on load instead of just on AJAX response.

@phalkunz phalkunz removed their assignment Nov 21, 2017
@chillu
Copy link
Member

chillu commented Dec 3, 2017

Since this has been around since 3.x times, I'll remove the milestone. @newleeland has already fixed this by redesigning the GridField search: silverstripe/silverstripe-cms#1777

@chillu chillu removed this from the Recipe 4.0.1 milestone Dec 3, 2017
@chillu
Copy link
Member

chillu commented Feb 11, 2018

Talked to @unclecheese, I'm OK with hacks to fix this, as long as it doesn't affect/change the PHP APIs. The current version of GridField will be rewritten in React anyway, with quite a different API surface most likely. Have a think about how this hack could affect popular third party GridField addons though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants