@peterhashair opened this Pull Request on September 15th 2021 Contributor

Description:

  • remove the segment selector.

File Changes

  • segmentgenerator.directive.less

    • update some CSS that not fits inside the modal box
  • Segmentation.js

    • removed unused javascript related to selector drop-down
    • update HTML to a hidden input to inherit the selected value
  • segmentation.less

    • fix the UI error on mobile view
  • _segmentSelector.twig

    • removed unused HTML UI
  • SegmentSelectorEditor_spec.js
    • removed related js Test.

Review

@tsteur commented on September 15th 2021 Member

@peterhashair nice! Could we instead of having a CSS solution that adds more css instead remove the HTML for that part?

@justinvelluppillai commented on September 16th 2021 Contributor

build js

@tsteur commented on September 21st 2021 Member

@peterhashair

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.

@peterhashair commented on September 21st 2021 Contributor

@tsteur sure thing, let me make more comments on the files, there are quite a few changes on the javascript may be confused.

@peterhashair commented on September 21st 2021 Contributor

@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.
image

@sgiehl commented on September 21st 2021 Member

@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.

@tsteur commented on September 21st 2021 Member

fyi we have an issue to improve the segment editor in https://github.com/matomo-org/matomo/issues/16646

@peterhashair commented on September 21st 2021 Contributor

@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.
image

@sgiehl commented on September 22nd 2021 Member

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:

dev matomo io_index php_module=CoreHome action=index idSite=1 period=day date=yesterday (1)
dev matomo io_index php_module=CoreHome action=index idSite=1 period=day date=yesterday (2)

With this PR it looks like this:

dev matomo io_index php_module=CoreHome action=index idSite=1 period=day date=yesterday (3)
dev matomo io_index php_module=CoreHome action=index idSite=1 period=day date=yesterday (4)

Personally I would prefer the first one, but maybe @tsteur and/or @mattab can make a decision on that.

@tsteur commented on September 22nd 2021 Member

In English it works better for me with the new changes 👍
image

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

image

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.

@peterhashair commented on September 23rd 2021 Contributor

@tsteur updated it, it should wrap the text in one line.

@tsteur commented on September 23rd 2021 Member

@peterhashair just tested it. And seems when you make the browser smaller there are certain sizes where it shows like in the screenshot below
image

vs before this was not the case and it was always showing correctly

@peterhashair commented on September 24th 2021 Contributor

@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.
image

@peterhashair commented on September 24th 2021 Contributor
@sgiehl commented on September 24th 2021 Member

@peterhashair Seems they are meanwhile removed. Last time I had a look at the diff they were still in place....

Powered by GitHub Issue Mirror