Fixes #728 - Use table-cell display for right label

[patrick-steele-idem]

Fixes #728

This PR includes a simple change to move the right-label span up one level so that it will be displayed as a table-cell instead of using float: right. This fixes layout problems by ensuring that the right label always has enough space reserved to avoid lines shifting down.

Before change:

After change:

After change, with a really long suggestion:

[patrick-steele-idem]

I just added some screenshots. Please let me know if you see any issues. Thanks for reviewing!

[winstliu]

This appears to have broken some specs - can you please investigate those failures @patrick-steele-idem? Thanks!

/cc @simurai

[patrick-steele-idem]

Any pointers how to visually test the failing tests to know what is happening and why the tests are failing?

[patrick-steele-idem]

@50Wliu @simurai I spent more time than I would like to admit investigating the failures, and while I was able to fix one of the tests, I am at a loss while the two tests are failing...

Since I was not able to visually see the editor when running the tests I took the time to reproduce the tests by creating the same provider that the failing tests were using. When I tested in my editor everything was working exactly as expected and the overlay is positioned exactly where it should be as shown in the screenshot below:

I'm really hoping someone else can take a look and let me know what I am missing. Is there some setting that I am missing that is impacting the test? Is there a way to see what the editor looks like when running the tests? Any help would be greatly appreciated. Thanks in advance.

[winstliu]

Is there some setting that I am missing that is impacting the test? Is there a way to see what the editor looks like when running the tests?

I don't believe so, sorry.

I'm afraid I can't help much with the tests either, but it looks like the failures start happening after you backspace past a space. Did you test that? I'm not quite sure based on your screenshot.

[patrick-steele-idem]

I'm afraid I can't help much with the tests either, but it looks like the failures start happening after you backspace past a space. Did you test that? I'm not quite sure based on your screenshot.

Yeah, that's exactly what I did to produce my screenshot. Unfortunately, the screenshot did not capture the cursor position (I probably should have done an animated gif in retrospect).

By the way, the test is only failing because of the change to move the right-label span up one level. The change to the autocomplete.less did not impact the tests, but the styles were changed because otherwise a horizontal scrollbar would show up in some situations.

What are your thoughts on resolving the failing test? The test itself seems fine and I am not seeing the problem visually which is what is frustrating...

[simurai]

I get the specs to pass when removing this + this.

I guess they both add some extra space to the calculation in the spec, even tough visually the position is correct.

[simurai]

Another issue is that when all 4 spans are filled up, it starts to scroll horizontally. It is already broken on master, there is just wraps.. hence this PR. 😄

It seems that the .icon-container isn't included in calculating the whole width. When doing .icon-container { display: none !important; and remove the .suggestion-list-scroller { margin-right: 10px; }, then there is no horizontal scrolling.

Edit: Actually the horizontal scrolling is caused because there is a max width 800px, so fix might be to just make the middle word element smaller so that the .icon-container still fits.

[simurai]

@patrick-steele-idem ok, there is now a PR patrick-steele-idem#1 that might help with this PR.

[patrick-steele-idem]

Update: I merged in some additional fixes from @simurai (thank you!) and that resolved the tests. I did some additional visual testing and made one more tweak to resolve a horizontal scroll bar when the right label was long. I think we are good to go, but please let me know if you still see any problems. Thanks!

[patrick-steele-idem]

Here's an updated screenshot that is using the latest code:

Everything appears to layout properly when there is an icon, left label, long text and a long right label.

[simurai]

I can't reproduce the "horizontal scrolling" when removing the margin-right: 10px;.

It looks like the 10px are for the scrollbar? Are you on OS X? I have the scrollbars only shown "when scrolling", but even when I turn them on permanently, it seems fine. It just shifts a bit over:

[patrick-steele-idem]

I am on OSX. It's very strange that you aren't seeing the issue. I've configured the font-size to be 13px and I am using the Hack font, but other than that my styles are pretty standard. I am using the Atom Dark theme. Here's two recordings to show the issue:

Without margin-right: 10px (bad horizontal scroll bar)

With margin-right: 10px (no horizontal scroll bar)

[designorant]

I have only had a glance at this as I cannot reproduce this exact behaviour yet, but wanted to share my concerns about this PR:

1. We seem to be mixing up em (paddings) and px (min/max widths) values now.
2. Source change rendered .word-container somewhat obsolete.
3. The max-width numbers don't add up to 800px, as they used to:

autocomplete-suggestion-list.select-list.popover-list {
  max-width: 800px;
}

autocomplete-suggestion-list {

  .left-label {
    max-width: 150px;
  }

  .right-label {
    max-width: 150px;
  }

  .word {
    max-width: 500px;
  }

}

With all this in mind I wonder whether it would be simpler to refactor the existing stylesheet to use Flexbox instead? That would likely improve the use of whitespace as well.

Related issues: #389, #566, #670.

[SkyzohKey]

👍 @designorant flexbox would solve the issue with something simple as

autocomplete-suggestion-list {
  display: flex;

  .left-label {
    flex: 0 0 25%;
  }
  .right-label {
    flex: 0 0 25%;
  }
  .word {
    flex: 50%;
  }
}

Using percents would also make the grid being responsive, not depending on an hard-coded max-width ;)

[simurai]

So if I understood it right, it goes something like this:

Flexbox would be nice if the proportions should be predefined (25% 50% 25%). Using a table layout is great if the columns should adjust automatically based on how much content gets added. In case that only the right-labels are very long, with flexbox they would get truncated, but with a table you could see more:

Flexbox:
| xxxx | xxx       | xx.. |
| xx   | x         | xx.. |

Table:
| xxxx | xxx | xxxxxxxxxx |
| xx   | x   |    xxxxxxx |

Buuut.. there are issues with the table growing uncontrollably wide. table-layout: fixed; would help, but then the auto adjusting doesn't seem to work anymore.. so the max-width to the cells got added to limit the table's width.

The px values in this PR don't add up because the left .icon-container should resize based on the font-size of the editor and therefore uses ems. And the width of the scrollbar gets defined by themes and can only be approximated. So the 430px (instead of 500px) for .word is just a number that should allow some extra space for those unknowns.

I agree that it feels a bit ugly, but maybe still an ok compromise for the feature it allows?

[simurai]

@designorant Source change rendered .word-container somewhat obsolete.

Yes, that could be removed. And even rename .word to display-text to match the API? Separate PR maybe fine too.

[simurai]

Ok, let's merge this since it fixes something that currently looks broken.

@patrick-steele-idem Thanks! 🙇