@peterhashair nice! Could we instead of having a CSS solution that adds more css instead remove the HTML for that part?
I think I understand why https://github.com/matomo-org/matomo/pull/17998/files#diff-04464652e4f40674780888b5ac96a5768d9b41c279a91d58df1094c9af9ab022L291-L304 and https://github.com/matomo-org/matomo/pull/17998/files#diff-05e5730fc6d27386e71d02ec01af30266f96593438ba2e504a8e900dccd0e0b5L39-L42 was removed since this is the part that removed the selector. Do you mind leaving a few comments in the PR in the files changed why there are quite a few other changes? This will help review the code and understand the changes as it's not quite obvious why they were done. This will make it also easier to test it when reviewing.
@sgiehl ah I haven't thought about the lanaguage, that's a hard one, russian is longer one, that actually broken every css... maybe we should put them into 2 lines. if that's ok.
@peterhashair That's something you should try to have always in mind. It's in most cases a bad idea to adjust the layout so it fits with some specific text. Guess this was the reason why the previous layout was chosen, where each text could wrap into the next line if it didn't fit in. I guess there might be a lot potential for improvements in the segment editor, like shorten some of the text, or moving those options somewhere else in the layout. But maybe we shouldn't do it as part of this issue, as it was basically only about removing the segment selection.
fyi we have an issue to improve the segment editor in https://github.com/matomo-org/matomo/issues/16646
@sgiehl sorry, I didn't think the text wrap, a really good thing to remember. For the moment, just updated the
inline-block toinline`, it should go to the next line when it's not fit the same line.
I'm actually not sure if the new layout looks better than the old one (with removed segment selector).
Using the layout from
4.x-dev, with hidden segment selection it would look like this:
With this PR it looks like this:
In English it works better for me with the new changes 👍
I would maybe just change it that when the setting goes over to the next line, that then the word
and also goes to the new line. I mean in screenshot below
The last word "und" on the first line should be in the second line. This should only happen though if it breaks over multiple lines. It should not be on the second line if everything fits in the first line anyway.
@tsteur updated it, it should wrap the text in one line.
@peterhashair just tested it. And seems when you make the browser smaller there are certain sizes where it shows like in the screenshot below
vs before this was not the case and it was always showing correctly
@tsteur thanks for pointing that out, fixed it. Also, I noticed the visit-in real-time widgets UI is broken on your screen as well.
@sgiehl Sorry to bother, regard those files, are they still there? I saw those in the PR
The screenshots are still there and should be removed:
@peterhashair Seems they are meanwhile removed. Last time I had a look at the diff they were still in place....
@peterhashair There are still a few UI tests failing. See https://app.travis-ci.com/github/matomo-org/matomo/jobs/539682400#L1182
Just checked the failures locally. Not fully sure, why the tests started failing, but I would assume, that the layout change somehow causes the autosuggest box to be displayed too far over the button to add an or condition, causing the segment editor to be closed when puppeteer tries to click it. Adding a blur might help. e.g. something like
await page.$eval('.segmentEditorPanel .segmentRow0 .ui-autocomplete-input', e => e.blur()); at the beginning of this test: https://github.com/matomo-org/matomo/blob/29819cc4497e2ba623392c2b90e3e4246c790e1b/plugins/SegmentEditor/tests/UI/SegmentSelectorEditor_spec.js#L88-L93
@sgiehl yeah, I am trying to figure out those 2 tests as well, something wrong with my PR images, the expected images are correct, I will update the test code a little bit to fix that one.
@sgiehl sorry, that was my bad, pushed the wrong one.