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

Fix double table border when no sorting or selection is enabled #7772

Merged
merged 2 commits into from
Sep 9, 2023

Conversation

ajnsn
Copy link
Contributor

@ajnsn ajnsn commented Aug 15, 2023

Fixes #7632

  • Changes have been thoroughly tested to not break existing functionality.
  • Visual changes are explained in the PR description using a screenshot/recording of before and after.

This is a follow up for #7636. It changes the top part of the table to work with the parent divide-y by removing the css hidden class assignment when ! $isSelectionEnabled and ! count($sortableColumns) to an if-clause around the whole div.

This is to hide the current double border within a grid table when you are not having selections enabled or any sortable columns.

As Dan already stated here, the hidden class assignment must be there for a reason. But I cannot find any. If the table has no bulk actions, sortable columns etc, the part of the table imHo will always be hidden?

Before

before
before-dark

After

after
after-dark
after-with-sort-bulk
after-with-sort-bulk-dark

@what-the-diff
Copy link
Contributor

what-the-diff bot commented Aug 15, 2023

PR Summary

  • Conditional Toggle in Interface
    • Implement a sort of 'switch' that shows or hides sections of the user interface, based on certain conditions. This adaptation makes for a neat and user-friendly interface.
  • Check-box Addition
    • Include a new checkbox to the interface that visually indicates whether a specific function (reordering) is enabled or not. This provides users a clear view of what actions are available to them.
  • Dynamic Sorting Feature
    • Enhance interface with a sorting function. Users can now choose how to order the columns—this flexibility empowers users to view data in a way that best suits their needs. The selection choices are generated automatically, ensuring the feature is always up-to-date.

@zepfietje
Copy link
Member

zepfietje commented Aug 15, 2023

IIRC, hidden is there for performance reasons or some issue that occurred in the past, Dan said.

@wychoong
Copy link
Contributor

fyi, its from one of my PR to solve an issue #5977

@danharrin
Copy link
Member

Can we replace divide-y with a class that accounts for hidden children? I feel like Tailwind does something very similar with space-*

@danharrin danharrin added bug Something isn't working ui labels Aug 15, 2023
@danharrin danharrin added this to the v3 milestone Aug 15, 2023
@zepfietje
Copy link
Member

@danharrin, that's what I used x-bind:hidden for in a couple of places, remember?

@danharrin
Copy link
Member

@zepfietje is that an option here?

@ajnsn
Copy link
Contributor Author

ajnsn commented Aug 15, 2023

Thanks @wychoong for the reference!

Yes, we could apply the "hidden" attribute, either via an if statement or via x-bind, it's described here.

I can also change the PR, let me know which option do you prefer, Examples:

  <div
      @class([
          'flex items-center gap-4 gap-x-6 bg-gray-50 px-4 dark:bg-white/5 sm:px-6',
          'hidden' => (! $isSelectionEnabled) && (! count($sortableColumns)),
      ])
      {{-- x-bind:hidden="{{ (! $isSelectionEnabled) && (! count($sortableColumns)) }}" --}}
      {{-- x-bind:hidden="! (@js($isSelectionEnabled) || @js(count($sortableColumns)))" --}}
      @if (! $isSelectionEnabled && ! count($sortableColumns))
          hidden
      @endif
  >

In any case I see a lot of @ifstatements all over the place, e.g. alone in this div the @if ($isSelectionEnabled && (! $isReordering)) and the @if (count($sortableColumns)) are direct children. So for me it would be the increase the readability a lot if there's a simple @if above. This hidden-approach is difficult to interpret, imHo :)

@zepfietje
Copy link
Member

It's hard for me to follow the exact issue and checking out the diff, but I guess simply using that Blade @if around everything is fine, as long as #5977 isn't reintroduced?

@ajnsn
Copy link
Contributor Author

ajnsn commented Aug 15, 2023

Afaik the structure in this PR was different. Here are screens of this PR without anything (even without search). Looks good to me.

nothing
nothing-dark

@danharrin
Copy link
Member

@if ((! $isSelectionEnabled) && (! count($sortableColumns))) hidden @endif

@zepfietje
Copy link
Member

hidden attribute is only needed if a divide- child is rendered but hidden using x-show, @danharrin.

# Conflicts:
#	packages/tables/resources/views/index.blade.php
@danharrin danharrin merged commit ad2d896 into filamentphp:3.x Sep 9, 2023
4 checks passed
@danharrin
Copy link
Member

Let's see if this presents any issues in the next release 👍

@ajnsn ajnsn deleted the fix-7632-2 branch September 11, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom table row borders
4 participants