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

Implemented ability to hide toolbar and entire data table when empty #1060

Conversation

gRoberts84
Copy link

For purposes of DRY, I am using the same data table component in multiple areas of the site, some of which don't need the toolbar etc (disabling individual components hides them but still keeps the wrappers) and some don't need to appear at all if there are no results to show.

In addition to this, I also added the ability to include a custom view before the data table, so you can add things like titles, that if combined with hiding when empty, shouldn't show either.

Happy to make as many changes as needed to make it acceptable.


All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests and did you add any new tests needed for your feature?
  2. Did you update all templates (if applicable)?
  3. Did you add the relevant documentation (if applicable)?
  4. Did you test locally to make sure your feature works as intended?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@lrljoe
Copy link
Collaborator

lrljoe commented Feb 23, 2023

Tested this briefly today, and have a few comments on it.

beforeComponentView
There's a new configurable area (before-tools), which is positioned just inside the wrapper in datatable.blade.php

    <x-livewire-tables::wrapper :component="$this">
        @if ($this->hasConfigurableAreaFor('before-tools'))
            @include(
                $this->getConfigurableAreaFor('before-tools'),
                $this->getParametersForConfigurableArea('before-tools'))
        @endif

Is there a reason you'd want another area just above the wrapper? See here for the pull request that added this functionality, it was only a week or so ago, so appreciate you've probably not seen it.
Not certain, but it may be better to use the configurable areas if you definitely want to implement another configurable area, just to keep things a little cleaner, although at some point the template file will just be full of configurable areas!

Hide on Empty
So a few issues here!
$rows contains other things such as the pagination method (if not using simplePagination), so it won't necessarily be empty. You'd need to adjust datatable.blade.php to have the following instead:

@if (!$this->getHideOnEmptyStatus() || ($this->getHideOnEmptyStatus() && $rows->count() > 0))

However!!! The big issue with this is if you have no rows as a result of a filter/query, then you're going to end up with the component disappearing at random, and if you've implemented more advanced caching for search/filtering options, then you may end up with a bit of an issue, particularly if you're just an end-user.

So I'd recommend that you adjust the wrapper to cater for if there's an active filter/search in place, maybe something like the following, but please do test more accurately, this will display the table if there's an active filter, a search value, or rows to display.

@if (
    !$this->getHideOnEmptyStatus() ||
        ($this->getHideOnEmptyStatus() &&
            (!empty($this->getSearch()) || 
            $this->hasAppliedFiltersWithValues() || 
            $rows->count() != 0)
         )
   )

You should also return something from the blade, otherwise it can be seen as poor practice, and will give a bit of an odd end-user experience!

It is also worth noting that there's already the following option available:

public function configure(): void
{
  $this->setEmptyMessage('No results found');
}

Perhaps, taking on board the above feedback around filters/search etc, a "Hide Toolbar on Empty" would be a good addition too as part of this?

@gRoberts84
Copy link
Author

Hey @lrljoe

Thanks for looking at my PR and providing feedback.

beforeComponentView
The reason for not using the existing before-tools, is that it is still within the main datatable component.

If I were then to want to hide the component for some reason (e.g. no results) then I'd also have to perform other queries to determine whether to hide what is showing before it (e.g. a title.)

Hide on Empty
Apologies, I based this on the @forelse that was being used that had an empty state to show the empty message.

Admittedly, I'm basing these changes on my use case, where I have a datatable to display all of the entries in a table and we also want to show the 10 upcoming and 10 past entries on a landing page/dashboard.

Initially I had copied the HTML that the component had generated and manually populated it with my data, but it makes sense to reuse the component and disable parts of it to produce the desired effect.

Although it would be possible, the intention would be to not Hide on Empty whilst allowing the user to filter/search. Maybe we could hide the toolbar if the user enables Hide on Empty?


Appreciate you taking the time to look at this for me.

Happy to discuss in more detail or make changes as and where needed.

@lrljoe
Copy link
Collaborator

lrljoe commented Feb 23, 2023

The reasoning behind the Before Component view makes sense to me!

Apologies if I wasn't clear on my point around hide on empty.

To replicate my point with your current code.

  1. Enable hide on empty on your table
  2. Go to an instance that has results
  3. Search or filter in such a way that you get no results. E.g search for loremipsum8888, instead of a "no result" message
    The table component will disappear.

This will be an issue if using setSearchDebounce for example, as if you make a typo when searching and don't correct in time then the component will vanish. If you have queryString enabled then a refresh of the page won't even return until you clear your queryString.

Whereas the version of the @if function that I posted for the datatable.blade won't do that. What my suggestion should do is:

  1. If HideOnEmpty is disabled - display the table, regardless
  2. If HideOn Empty is enabled - display IF
  • There is a Search Input (because to make a search input, the table must have previously been visible)
    OR
  • There is an Applied Filter (because to apply a filter, the table must have previously been visible)
    OR
  • There are results (as we want to see them!)

So, in theory, this'll hide if there's no initial results, but won't hide if there's no results due to a filter/query on the table (that initially had results)

Perhaps it may be an idea to move the beforeComponentView to above the HideOnEmpty, so that something is always returned, and this can then be used when the component is hidden (thus returning always returning something - optional as to what!)

Don't get me wrong, it's entirely possible that I'm wrong here and @rappasoft may be happy with the approach you've taken, just giving you my opinion! :)

@gRoberts84
Copy link
Author

Hey @lrljoe

Thanks for getting back to me.

I see your point if you were to have both the toolbar and hide on empty enabled.

In my use case, I have two instances of a datatable on my dashboard, showing 10 past and upcoming tasks.

Should the user that is logged in have no tasks to show for either or both, instead of the datatables collapsing and the content below moving up, it will show the No Result.

I have currently copied the generated HTML and created my own data table at the moment, however I'd rather use the datatable that shows the exact same information etc.

Thanks again

@lrljoe
Copy link
Collaborator

lrljoe commented Feb 23, 2023

Hey @lrljoe

Thanks for getting back to me.

I see your point if you were to have both the toolbar and hide on empty enabled.

In my use case, I have two instances of a datatable on my dashboard, showing 10 past and upcoming tasks.

Should the user that is logged in have no tasks to show for either or both, instead of the datatables collapsing and the content below moving up, it will show the No Result.

I have currently copied the generated HTML and created my own data table at the moment, however I'd rather use the datatable that shows the exact same information etc.

Thanks again

I actually have the same use case in one of my environments.
Instead of two tables, I set a public boolean variable to determine whether to show the past or the upcoming tasks. I display this to the user as a tab for the UI

It was well received by my users so may be worth an a/b test with yours.

My builder just has a when() function to determine which set of tasks to show.

If you'd rather publish the views and customise them as you've done in your fork, then you obviously can do, but just keep an eye out in case of any breaking changes! And use a trait to add your extra functionality.

I do like the idea of hiding the component completely for some use cases though. So do feel free to make the necessary adjustments to avoid impacting other use cases and continue with the PR, don't let my negativity stop you!

@gRoberts84
Copy link
Author

Hey @lrljoe

Thanks for getting back to me :)

I'll probably just revert to using my own component with the HTML that was ultimately generated by the datatable.

Apologies if I came across otherwise but I never saw any of your feedback as being negative.

The whole PR process is meant to work like this and believe me, you've been extremely helpful.

@lrljoe
Copy link
Collaborator

lrljoe commented Mar 5, 2023

Just wanted to check whether you've published the views and just made your amends there, then deleted the ones you haven't changed, or if you've done something different?

I do like the idea so I may pilfer your PR and make the necessary adjustments to get it functional when using filters.

@stale
Copy link

stale bot commented Apr 30, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Apr 30, 2023
@stale stale bot closed this May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Research wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants