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

Jump to bottom

Remove "add new segment" selector in segment editor #17998

Merged
merged 38 commits into from Sep 29, 2021
Merged

Remove "add new segment" selector in segment editor #17998

merged 38 commits into from Sep 29, 2021

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Sep 15, 2021 •
edited

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

@peterhashair peterhashair added the Needs Review PRs that need a code review label Sep 15, 2021
@justinvelluppillai justinvelluppillai added this to the 4.5.0 milestone Sep 15, 2021
@peterhashair peterhashair linked an issue Sep 15, 2021 that may be closed by this pull request
Remove "add new segment" selector in segment editor #17890
Closed
@peterhashair peterhashair modified the milestones: 4.5.0, 4.6.0 Sep 15, 2021
@peterhashair peterhashair added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Sep 15, 2021
@tsteur
Copy link
Member

tsteur commented Sep 15, 2021

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

@peterhashair peterhashair changed the title simple approach, hide segment by css. complete solution needs to alte… Remove segment selector Sep 16, 2021
@justinvelluppillai
Copy link
Contributor

build js

add no wrap to the display
edc7eca 
add no wrap to the display
@peterhashair
Copy link
Contributor Author

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

@tsteur
Copy link
Member

tsteur commented Sep 23, 2021

@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

fix the UI on different device
f9947bb 
fix the UI on different device
@peterhashair
Copy link
Contributor Author

peterhashair commented Sep 24, 2021 •
edited

@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

update images
d222245 
update images
@peterhashair
Copy link
Contributor Author

@sgiehl Sorry to bother, regard those files, are they still there? I saw those in the PR

image

The screenshots are still there and should be removed:

@sgiehl
Copy link
Member

sgiehl commented Sep 24, 2021

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

@peterhashair peterhashair modified the milestones: 4.5.0, 4.6.0 Sep 27, 2021
@sgiehl
Copy link
Member

sgiehl commented Sep 27, 2021

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

matomo/plugins/SegmentEditor/tests/UI/SegmentSelectorEditor_spec.js

Lines 88 to 93 in 29819cc

it("should add an OR condition when clicking on add OR", async function() {
await page.click('.segmentEditorPanel .segment-add-or');
await page.waitForFunction(() => !! $('.segmentRow0 .segment-rows>div:eq(1)').length);
await page.waitForNetworkIdle();
expect(await page.screenshotSelector(selectorsToCapture)).to.matchImage('add_new_or_condition');
});

@peterhashair
Copy link
Contributor Author

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

@peterhashair peterhashair changed the title Remove segment selector Remove "add new segment" selector in segment editor Sep 27, 2021
sgiehl
sgiehl requested changes Sep 28, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are changes on the expected screenshot file tests/UI/expected-screenshots/ViewDataTableTest_exclude_low_population.png, which seem unrelated to this PR, so might be good not to update it here.

plugins/SegmentEditor/tests/UI/SegmentSelectorEditor_spec.js Outdated Show resolved Hide resolved
plugins/SegmentEditor/tests/UI/SegmentSelectorEditor_spec.js Outdated Show resolved Hide resolved
@peterhashair
Copy link
Contributor Author

@sgiehl sorry, that was my bad, pushed the wrong one.

Peter Zhang and others added 5 commits September 29, 2021 17:47
sgiehl
sgiehl approved these changes Sep 29, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the ui screenshots and removed some unneeded css code. Now everything seems to be fine 👍

@sgiehl sgiehl merged commit d671dcd into 4.x-dev Sep 29, 2021
@sgiehl sgiehl deleted the matomo-17890-remove-segment-selector branch September 29, 2021 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsteur tsteur tsteur left review comments

@Findus23 Findus23 Findus23 left review comments

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.6.0
Development

Successfully merging this pull request may close these issues.

Remove "add new segment" selector in segment editor
5 participants
@peterhashair @tsteur @justinvelluppillai @sgiehl @Findus23