Skip to content

feat(filter): add filterContainer option to allow a column filter to be placed in column menu #6901

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

Merged
merged 1 commit into from
Dec 19, 2018

Conversation

caseyjhol
Copy link
Contributor

New behavior: http://next.plnkr.co/edit/nJbWFbLy7qutQslTQovF?preview

filterContainer has a default value of "headerCell" which is the same as existing behavior. A filterContainer value of "columnMenu" moves the filter into the column menu. This improves the appearance of the filter in situations where there are multiple filters for a single column (preventing the header from simply getting taller and taller) and offers a streamlined alternative to placing the filter directly in the header cell.

Fixes #3989.

…be placed in column menu

filterContainer has a default value of "headerCell" which is the same as existing behavior. A filterContainer value of "columnMenu" moves the filter into the column menu. This improves the appearance of the filter in situations where there are multiple filters for a single column (preventing the header from simply getting taller and taller) and offers a streamlined alternative to placing the filter directly in the header cell.

Fixes angular-ui#3989.
@caseyjhol
Copy link
Contributor Author

@mportuga Any thoughts on this? Not sure why the commit lint is taking over a week.

@@ -328,6 +333,14 @@

updateHeaderOptions();

if ($scope.col.filterContainer === 'columnMenu' && $scope.col.filterable) {
$rootScope.$on('menu-shown', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is $rootScope needed here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it might make sense to have tests that specifically change that setFilter works as expected when menu-shown event is triggered,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'menu-shown' event is emitted from the uiGridMenu directive, which is outside the uiGridHeaderCell directive, so $rootScope is needed to listen for the event from within uiGridHeaderCell. I'm open to suggestions if you think there might be a better way. We could perhaps move the filter stuff to another directive ('uiGrid'?).

@mportuga
Copy link
Member

@caseyjhol Overall it looks good. Just one question. And I am not sure why commit lint is being slow.

angular.module('ui.grid').directive('uiGridHeaderCell', ['$compile', '$timeout', '$window', '$document', 'gridUtil', 'uiGridConstants', 'ScrollEvent', 'i18nService',
function ($compile, $timeout, $window, $document, gridUtil, uiGridConstants, ScrollEvent, i18nService) {
angular.module('ui.grid').directive('uiGridHeaderCell', ['$compile', '$timeout', '$window', '$document', 'gridUtil', 'uiGridConstants', 'ScrollEvent', 'i18nService', '$rootScope',
function ($compile, $timeout, $window, $document, gridUtil, uiGridConstants, ScrollEvent, i18nService, $rootScope) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move rootScope to the start? It is more of a personal preference, but I like having all of the $ variables grouped together.

Suggested change
function ($compile, $timeout, $window, $document, gridUtil, uiGridConstants, ScrollEvent, i18nService, $rootScope) {
function ($compile, $timeout, $window, $document, $rootScope, gridUtil, uiGridConstants, ScrollEvent, i18nService) {

@@ -1,8 +1,8 @@
(function() {
'use strict';

angular.module('ui.grid').directive('uiGridHeaderCell', ['$compile', '$timeout', '$window', '$document', 'gridUtil', 'uiGridConstants', 'ScrollEvent', 'i18nService',
function ($compile, $timeout, $window, $document, gridUtil, uiGridConstants, ScrollEvent, i18nService) {
angular.module('ui.grid').directive('uiGridHeaderCell', ['$compile', '$timeout', '$window', '$document', 'gridUtil', 'uiGridConstants', 'ScrollEvent', 'i18nService', '$rootScope',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
angular.module('ui.grid').directive('uiGridHeaderCell', ['$compile', '$timeout', '$window', '$document', 'gridUtil', 'uiGridConstants', 'ScrollEvent', 'i18nService', '$rootScope',
angular.module('ui.grid').directive('uiGridHeaderCell', ['$compile', '$timeout', '$window', '$document', '$rootScope', 'gridUtil', 'uiGridConstants', 'ScrollEvent', 'i18nService',

@caseyjhol
Copy link
Contributor Author

@mportuga You want me to submit another PR to move $rootScope? I can work on some more advanced tests, too.

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

Successfully merging this pull request may close these issues.

2 participants