Skip to content

Added configurable List Table Column character limits#143

Open
tomsb wants to merge 2 commits into
Laravel-Backpack:mainfrom
tomsb:main
Open

Added configurable List Table Column character limits#143
tomsb wants to merge 2 commits into
Laravel-Backpack:mainfrom
tomsb:main

Conversation

@tomsb

@tomsb tomsb commented Nov 26, 2024

Copy link
Copy Markdown
Contributor

Extending the controller would be easy, but I feel that's overkill for this functionality.
What do you think?

@pxpm

pxpm commented Nov 27, 2024

Copy link
Copy Markdown
Contributor

Hey @tomsb thanks for the PR.

I think in this case extending the Controller is the right approach. In no time you will want another attribute, then someone wants other, and we will have created a bunch of configs for field attributes.

Most of the time, if not every time, you would want to add new settings etc.

One thing we could think about is to make this package publish a class, like SettingsControllerDefinitions with the defaults (fields, columns etc) defined, and you can just change/add/remove the settings fields there.

The pro is that the SettingsCrud in this package would read that class to define the fields/columns, so you don't need to overwrite the controller and bind your new controller.
The downside is that is a "different" way of doing things and it might look weird and overcomplicating something that's "easy" to do.

I am tempted to say that just overwriting the controller give you much more flexibility, plus it will look and feel like other backpack crud controllers.

What do you think ?

Cheers

@tomsb

tomsb commented Nov 27, 2024

Copy link
Copy Markdown
Contributor Author

Makes sense to just overwrite the controller. My main issue with it is that the default limit of only 32 characters for the description column isn't sensible - it's too short for a description. This could be hardcoded directly into the column definition?

Also, the LV translation somehow ended up in this PR - apologies for that.

@pxpm

pxpm commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

Hey @tomsb yes I agree with you. 32 limit for description seems a bit rough. I don't mind that the column definition had some higher limit, but we should consider if increasing that limit, to say, 100 characters, wouldn't force the table to have a scrollbar, or if responsive is enabled, to hide some columns. In case it does I think we should keep it 32. Let's see how far we can stretch the column before it changes the table layout 👍

Want to send a PR ?

Cheers

@tomsb

tomsb commented Dec 22, 2024

Copy link
Copy Markdown
Contributor Author

A 100-character limit might cause a scrollbar or hide columns (if responsive), especially with name and value columns maxed at 32 characters. I'm fine with this - scrolling is quicker than opening rows for full descriptions.

Also, I dislike responsive tables and always disable them because it's hard to know what's hidden on different tables and screen sizes - but that's a different topic.

Should I send a 100-character PR or just close this issue?

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