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

feature: selectable table columns #1693

Merged
merged 27 commits into from Apr 20, 2022

Conversation

jszobody
Copy link
Contributor

@jszobody jszobody commented Feb 23, 2022

This is a first rough stab at adding column selection to tables. This is what it looks like:

CleanShot_2022-02-22_at_18 36 20

To make a column selectable, add ->selectable() when defining a column. If any columns in a table are selectable, the action icon appears on the table.

I very closely followed the way filters are implemented, copied and pasted quite a bit of that code.

I'm looking for input on a number of items:

  1. Naming things is hard. I have terms like ColumnSelection and CanBeSelectable and isColumnSelectionVisible and I'm really not sure these are well named. Is there a better single-word term for this "column selectability" behavior anyone can think of?
  2. I decided not to use any of the existing visible/hidden functionality of columns. It seems we should leave that for situations where a developer wants full control over whether a column appears at all or not. This selectable stuff is completely separate.
  3. Currently when a column is ->selectable() it is hidden by default until you select it. Need a way to also indicate that it should default to visible, thoughts on a good method name for that?
  4. Does it make sense to add this behavior on a column-by-column basis? I initially thought so, but now I'm wondering if this is overkill. Would it make sense to just have a table-level flag that simply makes ALL columns selectable?
  5. Changing column selections triggers a livewire request. We COULD make this entirely client-side instead, by rendering all table columns and controlling the visibility with alpinejs. This would be faster. It would mean though that we can't save selections server-side, say, in session. Thoughts?
  6. Speaking of saving column selections: should this be handled in session server-side? Should we persist to localstorage client-side? (Related: are there any plans for a database table where filament can store user preferences/settings/etc?)
  7. I'll need help with the language string, right now it's still using tables::table.buttons.filter.label which obviously will need to change.

Feedback welcome. =)

@jszobody jszobody marked this pull request as draft February 23, 2022 01:29
@ryangjchandler
Copy link
Member

ryangjchandler commented Feb 23, 2022

Regarding storage of selected/visible columns, I'd opt for $persist with Alpine personally so that it exceeds the lifetime of the user's session. We could also make this an Alpine.js handled thing then too.

I'd prefer to see a table-level flag that makes columns selectable, as well as the method on the column that can be used to disable the functionality for that particular column. The inverse of what you have now basically.

@dmandrade
Copy link
Contributor

dmandrade commented Feb 23, 2022

I think the behavior is like a toggle to enable/disable or show/hidden. for me "selectability" refers to another type of action other than showing/hiding something.

I like the option of having a table-level method to enable the function globally and two column-level methods to change the default display and disable the feature.

@ryangjchandler
Copy link
Member

Toggleable doesn't roll of the tongue...

@jszobody
Copy link
Contributor Author

jszobody commented Feb 23, 2022

Ok so let's think through the DX, if we flip this to a table-level flag.

$table
    ->columns(...)
    ->filters(...)
    ->selectable() // Is this what we want to see?

My thinking is that by making the table selectable it will, by default, hide columns. I'm guessing that if a developer wants this feature, it's because they have a ton of columns. Let's treat 'not-selected' as the default for any column that isn't otherwise specified.

Perhaps the selectable method would take an integer of how many columns to display for starters:

public function selectable(int $initiallySelected = 1)

As for the individual column option, perhaps...

Tables\Columns\TextColumn::make('name')->alwaysSelected(),
Tables\Columns\TextColumn::make('email')->selected(),
Tables\Columns\TextColumn::make('phone')

Adding ->selected() to a column would then make it initially selected by default. Adding ->alwaysSelected() would lock the column in a selected state, cannot be unselected by the user (essentially disabling the feature).

Thoughts?

@dmandrade
Copy link
Contributor

dmandrade commented Feb 23, 2022

Toggleable doesn't roll of the tongue

Agree. I just use word "toggle" for enable/disable something

Example

$table->enableColumnToggle(bool | Closure)
$column->canToggle(bool | Closure)
$column->defaultToggleState(bool | Closure)

@jszobody
Copy link
Contributor Author

Ok this has been significantly reworked, based on my comment yesterday.

The simplest way to make use of the new column selection is to add the selectabe() method when defining a table:

$table
    ->columns(...)
    ->filters(...)
    ->selectable();

By default, this will auto-select the first column in the table, and hide the rest. You can pass in an integer to this method to change the default number of columns selected. ->selectable(3) will show the first three columns, for example.

You can also control selection on an individual column if you want. Two methods are available:

Tables\Columns\TextColumn::make('name')->alwaysSelected(),
Tables\Columns\TextColumn::make('email')->selected(),
Tables\Columns\TextColumn::make('phone')

The selected() method makes the column selected by default. The alwaysSelected() method also locks this, such that the user cannot un-select. This is what the above example looks like:

CleanShot 2022-02-24 at 11 25 33@2x

Note: if you explicitly set selection on any individual columns, the table-level selectable() count is ignored, column-level selection rules take precedence.

@dmandrade While I still have mixed feelings about the 'selectable' verbiage, I want to try and keep to a terse term if possible, rather than multi-word methods like enableColumnToggle. Just a preference I suppose.

@ryangjchandler Most of the code you commented on has now changed, feel free to re-visit. However, before getting too detailed with the code specifics, I'd like to hear your thoughts on this updated API and whether you like the high-level approach. I still haven't focused much on closure arguments and such, wanting to get the big picture in place first.

@jszobody jszobody marked this pull request as ready for review February 24, 2022 17:59
@danharrin danharrin added the enhancement New feature or request label Feb 24, 2022
@danharrin
Copy link
Member

Hey, I've just had a chance to review this and quickly fix some formatting things. Overall, its great, you're definitely on the right track.

However. I dont think the term "select" or "selectable" is suitable here. Since that could easily get confused with selecting rows that we used for bulk actions. I'm not sure of an alternative though... tbh I think I prefer "toggleable" but I'm open to other suggestions. Maybe toggleColumns() alwaysToggled() etc.

Also, persistence of toggled columns between page reloads didn't seem to work for me, not sure if its a bug or you haven't thought about it yet.

@danharrin danharrin marked this pull request as draft March 6, 2022 11:18
@joshbenham
Copy link

Hey, I've just had a chance to review this and quickly fix some formatting things. Overall, its great, you're definitely on the right track.

However. I dont think the term "select" or "selectable" is suitable here. Since that could easily get confused with selecting rows that we used for bulk actions. I'm not sure of an alternative though... tbh I think I prefer "toggleable" but I'm open to other suggestions. Maybe toggleColumns() alwaysToggled() etc.

Also, persistence of toggled columns between page reloads didn't seem to work for me, not sure if its a bug or you haven't thought about it yet.

Could you use Visible? Then you could have toggleVisibility() and alwaysVisible().

@danharrin
Copy link
Member

I think toggleVisibility() is clear, yeah. But we need to make sure we don't get mixed up with existing visible* column methods that deal with responsive columns.

@nkeena
Copy link
Contributor

nkeena commented Mar 29, 2022

Anything I can do to help get this PR merged? I would love to use this feature in a current project

@danharrin
Copy link
Member

Anything I can do to help get this PR merged? I would love to use this feature in a current project

Not sure if @jszobody wants us to take over or not, he's probably very busy. If you're interested though, you could maybe clone his branch and rename any terminology from "select" to "toggleVisibility". Might require quite a few changes but all of the real work to get the feature working has been done.

@jszobody
Copy link
Contributor Author

Sorry folks, yes I've been out of town lately and too busy to revisit this. I won't be able to put much effort in until after next week.

There are a few open questions in my mind that perhaps @danharrin can help clarify for me.

  1. Since there are already visible() methods on columns, it seems super confusing to use visibility for this behavior. Don't you think future devs are going to get those mixed up?

  2. If you're sure you like visibility, I'll go with it. But I'm unclear on the desired API. Would toggleVisibility() be at the table or column level? $table->columns(...)->toggleVisibility() feels awkward to me. I have a table-level switch to enable this behavior in general, and then a column-level switch to control whether it is toggled/selected by default or not. Interested in nailing down the precise API you think would be developer-friendly, before refactoring the code again.

  3. I have not implemented any persistence yet. Can you provide me some thoughts on how you think that should be handled:

    a) Server-side in session. Easiest to implement, obviously only preserves column selections within a specific session.

    b) Client-side in local storage. If we do this though, it might mean revisiting how this entire feature is currently designed. Right now it's the server where default toggling is specified. That means we'd initially render the table with certain columns toggled, then check local storage for stored column preferences, then trigger an immediate livewire request to modify the column toggles. That immediate second trip to the server seems undesirable, no? If we want to store the column selections in local storage, I think that might suggest handling the entire interaction of toggling/untoggling columns client-side. The server would render ALL columns, and in Alpine we'd hide any columns we don't want to see, and toggle those entirely client-side with no more LW trips to the server at all, and updating local storage on all toggle changes.

    c) Some kind of user-specific preference storage on the server. This is frankly my favorite, but I don't think Filament has anything to handle user preferences currently, and would need to introduce that first. If you thought that storing user preferences is something Filament needs to have in the near future, might be worth building that first (DB table migration, presumably) so then the table column selections can be stored there.

@danharrin
Copy link
Member

danharrin commented Mar 29, 2022

Hey, yes there is a visible() method on columns, but you could think of that type of visibility as "static" and not "toggleable". The premise remains the same, "visibility" controls whether the user can see the column in the table or not. In comparison, if we went with "selectable", that could be too easily confused with behaviour that controls whether the user can select data it in the table, which is already possible for bulk actions.

IMO, the API should just be on the columns and not $table, as we do similar for searchable(), sortable() etc. As long as at least one column is toggleable, the toggle button is there.

My proposal is:

$condition // if the column should be toggleable

Column::make()->toggleable(bool $condition = true, bool $isVisibleByDefault = true)

then you could use Column::make()->toggleable() or Column::make()->toggleable(isVisibleByDefault: false)

With regards to implementing persistence - lets stick to sessions for now. We use that in other areas, like storing the pagination settings.

@danharrin
Copy link
Member

Will let @danharrin tear this code apart, but functionality wise it's all up to scratch and close to what has been discussed above.

It's actually perfect, it looks like I just made tons of comments but I just added Table to the name of some Livewire methods so they're consistent with others. Also I passed in the width to the popover.

@ryangjchandler
Copy link
Member

@danharrin Sweet, will sort these tomorrow. 🎉

Copy link
Member

@ryangjchandler ryangjchandler left a comment

Choose a reason for hiding this comment

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

This all looks good to me now.

@danharrin
Copy link
Member

And it all works with the new changes?

@ryangjchandler
Copy link
Member

@danharrin Yeah, all good still. Just tested :)

@ryangjchandler ryangjchandler self-assigned this Apr 19, 2022
@danharrin
Copy link
Member

Just pulled this down for testing, looks like the columns are visible oppositely to what their checkbox says?

When the table first load, the columns should be hidden / visible based on $isToggledByDefault, and if a checkbox is checked the column should be visible. Right?

Screen.Recording.2022-04-19.at.13.49.24.mov

@ryangjchandler
Copy link
Member

Oh yeah, I've been thinking of this the wrong way. Just need to invert the isToggled logic.

@ryangjchandler
Copy link
Member

Actually, I'm not sure now... to me you're toggling the column so the checkbox being checked is almost indicating that you've toggled it.

Happy either way though!

@danharrin
Copy link
Member

"Toggling" to me sounds like the process of showing/hiding the column, not an indication of whether the column is visible or not.

To me, I think it makes more sense if checked = visible, as on the original video #1693 (comment)

@danharrin danharrin marked this pull request as draft April 19, 2022 15:54
@ryangjchandler
Copy link
Member

Yeah, cool. I can change that in a bit.

@saade
Copy link
Member

saade commented Apr 20, 2022

Don't you guys think that the view-boards icon looks better representing what the button does?

image

@danharrin
Copy link
Member

I do @saade

@danharrin danharrin marked this pull request as ready for review April 20, 2022 19:56
@danharrin danharrin merged commit 262836a into filamentphp:2.x Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants