Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-17661 Pass ProfilePopover hide through to plugin components #3411

Merged
merged 5 commits into from
Aug 29, 2019

Conversation

MatthewDorner
Copy link
Contributor

Summary

Pass this.props.hide to Pluggable for Popover User Actions and Popover User Attributes plugin components, so the components are able to hide the popover.

Ticket Link

Fixes mattermost/mattermost#11842

@hanzei hanzei added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Aug 11, 2019
@hanzei hanzei requested a review from prapti August 11, 2019 10:56
@hanzei hanzei added this to the v5.16.0 milestone Aug 11, 2019
Copy link
Member

@lieut-data lieut-data left a comment

Choose a reason for hiding this comment

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

Thanks, @MatthewDorner!

Would you also be open to helping make a change to our https://github.com/mattermost/mattermost-plugin-demo to illustrate this new functionality?

@MatthewDorner
Copy link
Contributor Author

yeah, I can do that.

Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

Thanks @MatthewDorner, looks good to me

@saturninoabril saturninoabril removed the 2: Dev Review Requires review by a core commiter label Aug 15, 2019
@MatthewDorner
Copy link
Contributor Author

Not sure if you're waiting for the https://github.com/mattermost/mattermost-plugin-demo PR before merging this one, but I should have it ready sometime this week.

@prapti
Copy link
Contributor

prapti commented Aug 22, 2019

@MatthewDorner Could you please add unit tests for the PR. That'd be great!

@MatthewDorner
Copy link
Contributor Author

MatthewDorner commented Aug 23, 2019

Can you provide any guidance on where these tests should go? I see tests in a lot of different places in the repository and I'm still new to the project.

I guess I should add it to the Pluggable component's PropTypes as non-required, at least. Should it somehow be tested in the tests for ProfilePopover?

With some of the pluggable component types, as you can see in https://github.com/mattermost/mattermost-webapp/tree/master/plugins, there are container components, but there aren't any for the popover plugin components.

@saturninoabril
Copy link
Member

@MatthewDorner Pluggable component is so generic where props are not exactly define. Since this implementation is specific to ProfilePopover component, I suggest to have the test on itself (profile_popover.test.jsx), checking whether props, including hide is passed into the Pluggable component. Something like:

import Pluggable from 'plugins/pluggable';
...
    test('should match props passed into Pluggable component', () => {
        const hide = jest.fn();
        const status = 'online';
        const props = {...baseProps, hide, status};

        const wrapper = shallowWithIntl(
            <ProfilePopover {...props}/>
        ).dive();

        const pluggableProps = {
            hide,
            status,
            user: props.user,
        };
        expect(wrapper.find(Pluggable).first().props()).toEqual({...pluggableProps, pluggableName: 'PopoverUserAttributes'});
        expect(wrapper.find(Pluggable).last().props()).toEqual({...pluggableProps, pluggableName: 'PopoverUserActions'});
    });

@MatthewDorner
Copy link
Contributor Author

Anything else?

Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

Thanks @MatthewDorner, looks good to me. Please update to master so we can merge.

@prapti for your review

Copy link
Contributor

@prapti prapti left a comment

Choose a reason for hiding this comment

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

Checked for unit tests.
Will be tested post merge into master.
Removing QA Review label.
Approving.

@prapti prapti removed the 3: QA Review Requires review by a QA tester label Aug 29, 2019
@hanzei hanzei added the 4: Reviews Complete All reviewers have approved the pull request label Aug 29, 2019
@hanzei hanzei merged commit ea25697 into mattermost:master Aug 29, 2019
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Sep 17, 2019
skheria pushed a commit to uber-archive/mattermost-webapp that referenced this pull request Oct 3, 2019
skheria pushed a commit to uber-archive/mattermost-webapp that referenced this pull request Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
6 participants