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

Fixed #2839 by creating an empty first column #2891

Closed
wants to merge 1 commit into from

Conversation

promatik
Copy link
Contributor

@promatik promatik commented Jun 1, 2020

This may sound like a bodge, but I'll try to prove my point on how this first empty column is important 😅

When I start to look at the #2839 issue I tried to solve it with javascript, soon I realized it would be very hard because the button were being added as an inner text of the first column, some times the first column was a text (with href), sometimes it was the plus sign to the details row, or even the checkbox for bulk editing.

I found out this solution because at first, I didn't notice the problem because the CRUD I was using as an example had enabled bulk actions, which has a blank_first_column, therefore the modal trigger was being added to this first empty column (also, by always adding this blank first column it's no longer necessary on bulk actions).

This first empty column allows it to handle both plus button to details row and the data tables button to the modal. I made some changes to the css so that column is not going to appear if there is neither one. I also made small changes to the css to align the checkboxs:

image

There was also unnecessary padding on non ordering columns, causing odd spacing on the table
image


Before:

image

image

Now:

image

image

image

image

image


Before (crud with few columns):

image

Now:

image


There is only one downside on this, there's always the empty first row, even when it's not needed.
image
Anyway, I think it worth 🤷‍♂️

I think I tested all the possibilities, everything's working nice.
This is not a breaking change, but I think @tabacitu needs to give his opinion on this.

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@pxpm
Copy link
Contributor

pxpm commented Jun 8, 2020

Hello @promatik

Thank you very much for this solution. Indeed me and @tabacitu discussed it and we could agree that the solution would be something along the lines of always using a first column, but we didn't figured out yet the best way to display it correctly.

I think you did a great job here. 🍾 🕺 🍾

Thanks again \o

@pxpm pxpm self-requested a review June 8, 2020 08:40
@promatik
Copy link
Contributor Author

promatik commented Jun 8, 2020

Glad I could help @pxpm!

@tabacitu
Copy link
Member

Thanks a lot for taking the time to do this @promatik ! And for the excellent details in the PR - impressive. I've just tested it and it works GREAT. Only had some minor issues (because of first-child the details row and the content of the table when empty did not have any padding), but I fixed them in #2939 . I'll close this PR in favour of that one, let's move the conversation there, since that one is based on a branch - and we can all make changes to it (if needed).

The big problem we're having now though (mentioned in that PR too) is that this constitutes a breaking change, because the changes in list.css are not backwards-compatible. The people that upgrade won't have the proper css, because that file doesn't get published, so we can't push this as a patch update (4.1.9), we'll have to wait until 4.2.0 to merge it, unfortunately, which is in September most likely 😞 ...

I wish there was a way around this, but I don't see one - the initial bug (#2839 ) is minor, so we can't justify slightly breaking all people's tables, in order to fix it. In the future maybe we auto-publish the CSS on composer update, so that we don't have this problem (see #2246 ), but that's a tough decision in itself.

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.

4 participants