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

Add ability to use Nayjest/Grids without a database call and also add methods to allow for hiding and showing columns based on screen size #118

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

reddinan
Copy link

@reddinan reddinan commented Feb 7, 2016

Thanks for making this package it's been very helpful.
I wanted to be able to use the table builder function without making an additional database call as the data already exists in my application and a second call is redundant.

I created a new DataProvider called CollectionDataProvider that accepts an Illuminate\Database\Eloquent\Collection object instead of a query.
The code itself is essentially copied from EloquentDataProvider so it shouldn't be too hard to understand.
Sorting is a bit weird with this Collection as it seems to be done via the query, but this wasn't a problem for me since my data was already pre sorted to begin with.
I also added some tests for the CollectionDataProvider, though there weren't any examples to work off of so they are quite basic. Essentially they just check to make sure no methods throw an error

I also added the ability to hide columns based on screen size. This method simply adds the bootstrap class hidden-* to both the header and the row column items when building the table. Here is an example of how to use it

    # Instantiate & Configure Grid
    $grid = new Grid(
        (new GridConfig)
            # Grids name used as html id, caching key, filtering GET params prefix, etc
            # If not specified, unique value based on file name & line of code will be generated
            ->setName('my_report')
            # See all supported data providers in sources
            ->setDataProvider(new CollectionDataProvider($result))
            # Setup table columns
            ->setColumns([
                # simple results numbering, not related to table PK or any obtained data
                new IdFieldConfig,
                (new FieldConfig)
                    ->setName('title')
                    # will be displayed in table heder
                    ->setLabel('Title')
                    # sorting buttons will be added to header, DB query will be modified
                    ->setSortable(true)
                    # hide column on mobile
                    ->hideXs()
                    # hide column on small screen
                    ->hideSm()
                    # hide column on medium device
                    ->hideMd()
                    # hide column on large will also work

            ])
            # Setup additional grid components

    );

@reddinan
Copy link
Author

reddinan commented Feb 7, 2016

The tests are failing because it needs to be installed in a laravel application and the style checks seem to be not passing, but a quick look shows that it was also the case for all other pull requests so let me know if that's an issue.

@Nayjest
Copy link
Owner

Nayjest commented Feb 9, 2016

Hi Andrew,
Thanks for contribution.
Regarding CollectionDataProvider -- At all it's good idea & I can accept such contribution, but I need to test it with Laravel 4.

Also, I already have ArrayDataProvider in presentation/framework.

Regarding hideXs / hideSm -- I can't merge it becouse generally grids was designed to work without twitter bootstrap too. I think, this is case when better to leave it for customizations outside main repository.

Regarding tests with Laravel, see how it's done here: https://github.com/presentation-framework/laravel/tree/master/tests there is "laravel/framework" dev-dependency & specific bootstrap for tests (it's a draft, some code may be not working).

And for future, please, create separate pull-requests for different features, becouse if maintainer refuse one feature, it's not possible to merge another features from samerequest.

Pull request must contain no code align / ident changes for code not related to new features.

@reddinan
Copy link
Author

reddinan commented Feb 9, 2016

Thanks for the information about Laravel tests. I couldn't find any information on how to properly create them except for this [https://websanova.com/blog/laravel/creating-a-new-package-in-laravel-5-part-5-unit-testing] which seemed very clunky.

You mention an "ArrayDataProvider" but the reason I wrote the code for the "CollectionDataProvider" class is because I couldn't find anything related to this in the codebase. There is an "ArrayDataRow" but no ArrayDataProvider. Is it possible it disappeared after a pull request or something?

As for the hideXs, Sm, etc, it actually does not require bootstrap, it simply adds the hidden-* class to each column header and data row. If you are not using bootstrap you would simply have to add this css manually but I can understand why you would not want this. What if I modified this function to add "style" attributes instead?
I have not tested Laravel 4.0 but it should work perfectly fine as the code is essentially the same as the EloquentDataProvider class without the database call.

Edit: I just did some research and you cannot inline media queries. Today I learned. The same functionality could be created by adding a simple CSS file, which would still fulfill the requirement of working without Twitter Bootstrap

@Mopster
Copy link

Mopster commented Nov 9, 2016

I'm looking for a simple PHP Array data provider so I'm trying out this one. While testing I came across the following issue:

On https://github.com/Nayjest/Grids/blob/master/src/Components/ShowingRecords.php#L38 it will call $paginator->total() but that method doesn't exist, however $paginator->count() works.

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.

3 participants