Tradeshift / tradeshift-ui

The Tradeshift UI Library & Framework
https://ui.tradeshift.com
Other
33 stars 45 forks source link

[Tabel UI] Checkboxes cells are not align with the rows on resize to smaller resolutions #343

Closed MirelaPaun closed 6 years ago

MirelaPaun commented 7 years ago

Choose the section that applies to you and use the template to help us figure out how we can help you.


Bug report

Use the bug label

Tradeshift UI version affected

v7.0.0

Expected Behavior

Checkboxes cells should be align with the rows on all resolutions.

Actual Behavior

Checkboxes cells are not align with the rows on some smaller resolutions (for example: laptop resolution 1440x900).

Steps to reproduce

Display table with some rows and checkboxes and view the table on different windows sizes.

Screenshots (optional)

issue-ui-table not-issue-ui-table

lloydhumphreys commented 7 years ago

Could you add some simplified stylesheet so JMO can try and reproduce? :)

MirelaPaun commented 7 years ago

Just create a table and add a <br> in a cell in the <p> tag, something like this:

<td id="key329946725-cell-0-0" class="border-left-none border-right-none" data-index="0" style="width: 80px;"><div class="ts-table-cell"><p>Goods<br>Receipt</p></div></td>

and it should be easier to reproduce.

You can access https://sandbox.tradeshift.com/#/apps/Tradeshift.DocumentManager/ and edit a <td>.

wiredearp commented 7 years ago

OK, @MirelaPaun, I believe that this is probably caused by

  1. A misconfiguration of the Table where column wrap is not applied
  2. A bug that causes the Table to appear like it was configured correctly

:information_source: I think you can fix the bug if you specify wrap: true on the columns whose cells contain more than one line of text, so on the "Document ID" and the "Amount" cols.

Alright. As mentioned in http://ui.tradeshift.com/#components/table/layouting.html, you must specify the wrap property multiline columns. This instructs the Table to measure the height of all the rows on each layout update, which is otherwise avoided since it is a costly procedure for the browser. Without wrap, the table can assume a standard height cell height (of one line) and that is what you see in the fixed selection column, the one that doesn't fit the other columns.

Now, normally you would only be able to see the first line of text in each cell if you forget to wrap the column, because the second line would be hidden by overflow. You would then (normally) read the documentation and figure out that you should wrap the column. But the images in the first column expands the overflow: hidden zone so that you can see all the lines and you have no way of figuring this out :cry:

So it's really a bug in the way we have implemented images in the Table.

But before we proceed: Does it work if you wrap the columns in question?

MirelaPaun commented 7 years ago

Thank you @wiredearp for your answer.

I added wrap: true on the DocumentID and Amount columns but the table looks like this: checkbox_big_space

I have a big space because in DocumentID cell I inserted 2 <p>, one <p> for document type and one <p> for date. I did this because I had different styles for document type and date in that cell and I didn't find other solution for different styles in the same table cell.

To fix the big space from DocumentID column, I have to remove the margin from that <p>, something like: .document-id .ts-table-cell p { margin: 0px; } . Removing that margin, my bug is fixed: checkbox_fixed

What do you think about this fix?

wiredearp commented 7 years ago

Just to be clear: You can then confirm that the cells in the (first) selection-column are aligned correctly with the other cells - also in smaller resolutions - when you specify wrap on at least one column?

About the <p> elements: According to our calculations, there should only be multiple <p> in the cells when the markdown contains at least two \n\n newline characters. If you only enter one \n newline, the content should be rendered with a <br> instead and so your CSS modification would not be needed. Not that there is anything wrong with it [1]

[1] I think in general it's always OK to overwrite component margins with zero 0 because that means that we are free to add a default margin. This way, the components margin becomes a consistent 22px whenever it should have a margin; and also a consistent 0px whenever it should not have a margin. If we don't add this default margin, it will end up with an inconsistent margin of 33px or 10px or 25px because the apps will try to add it themselves; and sometimes using a padding instead of a margin (which doesn't "collapse vertically" and thus becomes even more inconsistent when the number of components increases). CC @dsp and @bomouridsen with regards to the Netflix layout strategy.

MirelaPaun commented 7 years ago

Yes, I can confirm that the cells from checkbox column are aligned correctly when using wrap: true on DocumentID column. I tested on my desktop resolution and in chrome mobile emulator and it works.

Regarding the <p> elements, it's true that I use \n\n for DocumentID column because I want 2 <p> elements there and that's why I need to have margin: 0px.

sampi commented 6 years ago

Can't be reproduced. Closing.