Don't close statusmenu on header click

[iOSGeekster]

Summary

Fixes that the status dropdown closes, when clicking the "header"

I havn't added test as of yet, because I think it would be good to get some feedback on the potential fix first.

I'm not sure it's the optimal approach... Thing is that the click event bubbles all the way up to the menu, which then closes the dropdown. So I tried the make the same approach as we do in menu_group, where we disabled click for the dividers.

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-39742

NONE

[mattermod]

Hello @iOSGeekster,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

[devinbinnie]

LGTM! Thanks @iOSGeekster!

[iOSGeekster]

Would it make a sense to add a E2E test that verifies this? As I see it, it can't be tested isolated in submenu_item.test.tsx, but I could be mistaken. So maybe it would make sense to create a integration test with cypress, but if that is the case, it should probably be connected to at test case or something like that?

[michelengelen]

LGTM

[michelengelen]

@iOSGeekster adding E2E for this might make sense.

Although I just had an idea that would make the implementation and testing via Jest a bit easier.

Why not remove the event.stopPropagation if-block from the onClick handler and instead add a ternary to the onClick property like this?

return (
    <li
        className={classNames(['SubMenuItem MenuItem', {styleSelectableItem}])}
        role='menuitem'
        id={id + '_menuitem'}
        ref={this.node}
        onClick={this.props.isHeader ? this.onClick : null}
    >
        // ...
    </li>
);

That way you could test for the function to being called in the unit test instead of writing a cypress test for that.

WDYT?

cc/ @devinbinnie

[iOSGeekster]

@iOSGeekster adding E2E for this might make sense.

Although I just had an idea that would make the implementation and testing via Jest a bit easier.

Why not remove the event.stopPropagation if-block from the onClick handler and instead add a ternary to the onClick property like this?

return (
    <li
        className={classNames(['SubMenuItem MenuItem', {styleSelectableItem}])}
        role='menuitem'
        id={id + '_menuitem'}
        ref={this.node}
        onClick={this.props.isHeader ? this.onClick : null}
    >
        // ...
    </li>
);

That way you could test for the function to being called in the unit test instead of writing a cypress test for that.

WDYT?

cc/ @devinbinnie

I think that's a good idea. I'll go ahead and make those modifications.

[iOSGeekster]

Hmm... just off the bat, it seems like the stopPropagation() is needed. Or else the click event will bubble up to the status menu itself, (and not just the submenu) and toggle the menu, making it disappear.

[michelengelen]

@iOSGeekster in that case we could setup a new helper function that prevents that from happening and pass it instead of null in the condition. But that is something the webapp team needs to decide since this would mean to create a new pattern that is not being used as of now.

EDIT: Thinking of it now I think it might be a little overkill to add that only to get testing a bit more convenient.

Thoughts @hmhealey ?

[hmhealey]

If we're adding a test for this, I think I'd prefer an E2E test anyway since we'll know 100% that this does what we want (ie not closing the menu). If we're changing the code in that way, I feel like we're testing that the code does what it says, not that the code does what we actually want to.

We could still change how the code works though. I'm 0/5 on that.

[michelengelen]

yep ... agreed! After sleeping over it it does feel like we are adding something out of convenience. So E2E might be best here as @hmhealey says.

[iOSGeekster]

That sounds like a good idea. Does it need at test-case to be created? Most of the E2E test are tied up to at test-case number, ie. MM-XXXX

[michelengelen]

@iOSGeekster that sounds like a question for @jgilliam17 or @saturninoabril ... Could one of you help out here please? 🙇🏼‍♂️

[saturninoabril]

@iOSGeekster Please go ahead and write E2E with the test scenario you have in mind then we'll validate and generate the corresponding test case ID.

[iOSGeekster]

@iOSGeekster Please go ahead and write E2E with the test scenario you have in mind then we'll validate and generate the corresponding test case ID.

Cool! Will do, and ping you once it's ready. Thanks guys 🙌

[iOSGeekster]

@saturninoabril I've added a simple e2e test, that validates it.

[iOSGeekster]

/update-branch

[michelengelen]

LGTM

[jgilliam17]

Thanks @iOSGeekster
I added the test case reference number
@saturninoabril I ran the E2Es, but not able to find this test. I can re-run after the reference number is updated. Any suggestions? Disregard, I opened the wrong report. This run is still pending

[jgilliam17]

E2E report https://mattermost-cypress-report.s3.amazonaws.com/90559-311aedb-ee-ent-webapp-pr-9363-mm-ee-test:311aedb/mochawesome.html

Thanks @iOSGeekster
Tested, looks good to merge.

• Verified DND time options header no longer closes on click. Test created in Zephyr and reference number suggested.

[saturninoabril]

Thanks @iOSGeekster, LGTM! And thanks @jgilliam17 for adding the test case!

[mm-cloud-bot]

Test server destroyed

[mattermod]

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

[mm-cloud-bot]

Test server creation failed. See the logs for more information.

[mattermod]

Trying to auto merge this PR.

[mattermod]

Pull Request successfully merged
SHA: fbf5f76