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

Long unbroken text values in list columns override explicit widths #4618

Closed
ghost opened this issue Sep 12, 2019 · 25 comments
Closed

Long unbroken text values in list columns override explicit widths #4618

ghost opened this issue Sep 12, 2019 · 25 comments

Comments

@ghost
Copy link

ghost commented Sep 12, 2019

Basically someone has added a PR and merged it into the develop branch and not properly tested it.

Someone has now added this to all tables:

word-break: break-word;
word-wrap: break-word;

So now all the words in tables are cut up and not reading correctly and you haven't fixed the basic issue of making a table responsive!

Here is a correct solution for tables in October backend:

<div style="overflow-x:auto;">
  <table>
    ...
  </table>
</div>

Then remove the bad css code please:

word-break: normal;
word-wrap: normal;

I believe the pr creating this issue is here: #4236

Taking this one step further:

Because people are lazy to add this to their tables, you can inject the HTML wrapper into the dom.

I have already done this to fix my plugins:

$(function () {
    // Wrap the table with a container with css
    $(.form-control table').wrap('<div style="overflow-x:auto;"></div>');
});

p.s. If people add css changes can they try to isolate their css code to specific elements and not every single form element, please!

@w20k
Copy link
Contributor

w20k commented Sep 12, 2019

It's not that PR @ayumihamasaki2019 - it's related to preview only.

@bennothommo
Copy link
Contributor

@ayumihamasaki2019 @w20k I think it was this PR: #3834

@ghost
Copy link
Author

ghost commented Sep 12, 2019

@w20k @bennothommo sorry, wrong pr mentioned, I did a quick search.

@w20k
Copy link
Contributor

w20k commented Sep 12, 2019

@bennothommo hm...yeah, PR is a bit strange. I mean he had Chrome issue only, and he just added word-break.
Wierd...

@ghost
Copy link
Author

ghost commented Sep 12, 2019

@bennothommo @w20k

Any recommendations where to add my javascript repsonsive fix?

  • Will create a pr to fix this now. 👍

@bennothommo
Copy link
Contributor

@ayumihamasaki2019 I'd rather not have a JS fix if possible. Is there any way this can be fixed with just CSS? Perhaps using a @media override to remove word-break on mobile devices where it's likely to make the list look like crap?

@w20k
Copy link
Contributor

w20k commented Sep 12, 2019

@ayumihamasaki2019 please don't, we need to fix from the core not to add hack. Sorry, to say, but your approach is just fixing current and incorrect behavior.

@bennothommo mind if I rever the PR, and look at it. How original issues could be fixed?

@bennothommo
Copy link
Contributor

@w20k I think the original fix is the correct one for their particular issue, it's just that it becomes ugly when space is constrained.

@w20k
Copy link
Contributor

w20k commented Sep 12, 2019

@bennothommo I mean it's correct only when you have a long URL, not by default.

@bennothommo
Copy link
Contributor

@w20k Fair point - yeah, go ahead and revert it. We'll have to tackle the long URL/string issue another way I think.

@ghost
Copy link
Author

ghost commented Sep 12, 2019

@bennothommo

The hack has to be a container like a <div> with that css code wrapping the table element.

If you have images in a table column they are cut in half with that pr.

@w20k

Please investigate further and let me know.

p.s. a working example of my hack is found here: https://www.w3schools.com/howto/tryit.asp?filename=tryhow_css_table_responsive

(resize the test screen and you will see the table become responsive)

@bennothommo
Copy link
Contributor

@ayumihamasaki2019 I didn't think word-break affects images?

@ghost
Copy link
Author

ghost commented Sep 12, 2019

@bennothommo images coming from background-image css.

@bennothommo
Copy link
Contributor

@ayumihamasaki2019 Oh right, well that's going to be an issue regardless when working with constrained screen widths. :)

@w20k
Copy link
Contributor

w20k commented Sep 12, 2019

@bennothommo can't be reverted automatically. Would have to look at it at home :)
Or if you have a minute to fix it @bennothommo. Would appreciate your help :)

bennothommo added a commit that referenced this issue Sep 12, 2019
Reported issues with tables with constrained widths breaking content too much.

Refs: #4618
@bennothommo
Copy link
Contributor

@w20k Reverted: 5f0da7e

@LukeTowers LukeTowers added this to the v1.0.460 milestone Sep 12, 2019
@LukeTowers LukeTowers changed the title Tables in backend are all broken due to a pr being merged Long unbroken text values in list columns override explicit widths Sep 12, 2019
@w20k
Copy link
Contributor

w20k commented Sep 12, 2019

The fix is simple.

We should add table-layout: fixed; and note it somewhere in the docs for longs string use `cssClass: list-cell-style-long-string:

list-cell-style-long-string: `word-wrap: break-word; word-break: break-all;`

or just word-break: break-all;

@Samuell1
Copy link
Member

I had same issue it was breaking everything in columns then i just used word-break: normal; on tables in backend to quick fix it.

@ghost
Copy link
Author

ghost commented Sep 12, 2019

All I'm gonna say is this: whatwg/html#4908

@bennothommo
Copy link
Contributor

@w20k Doesn't table-layout: fixed just simply lock the table to the specified column widths? I think it's better auto-sizing the columns based on content, which has been the default behaviour so far. Adding an extra class to the table to optionally enable word-wrap might be a good idea, as then the ball is in the court of the developer to decide if it's worth enabling or not.

I also think @ayumihamasaki2019's idea of a scrollable container is a good one, although it needs to be done in the widget templates, not as a JS fix.

@Samuell1
Copy link
Member

@bennothommo table is already scrollable, if you hold on labels in table

@ghost
Copy link
Author

ghost commented Sep 16, 2019

Just tested @Samuell1 comment out, I did get the list widget to scroll, it's not labels its the <th> elements.

@bennothommo

But it's very confusing and not a good design. It would be better to change the mouse cursor from pointer to grab icon.

Current setup in list widget

All table hover is cursor: pointer;

1

Changing cursor to grab in list widget

Only table header is cursor: grab and rest is cursor: pointer;

2

Though, I think a better design could be done...

LukeTowers pushed a commit to octoberrain/system that referenced this issue Sep 19, 2019
Reported issues with tables with constrained widths breaking content too much.

Refs: octobercms/october#4618
@LukeTowers
Copy link
Contributor

@ayumihamasaki2019 feel free to submit a PR for the grab cursor icon, I like that as a good minor improvement in the meantime.

@ghost
Copy link
Author

ghost commented Sep 19, 2019

Done. 👍

@daftspunk
Copy link
Member

Fixed by #4629

@bennothommo bennothommo removed this from the v1.0.460 milestone Sep 26, 2019
daftspunk pushed a commit to octoberrain/system that referenced this issue Nov 4, 2019
Reported issues with tables with constrained widths breaking content too much.

Refs: octobercms/october#4618
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants