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

MM-15943 render divider before plugin menu items #3264

Merged
merged 5 commits into from Aug 22, 2019
Merged

MM-15943 render divider before plugin menu items #3264

merged 5 commits into from Aug 22, 2019

Conversation

janvt
Copy link
Contributor

@janvt janvt commented Jul 26, 2019

Summary

Render divider between normal menu items and plugin menu items on dot menu.

Ticket Link

Fixes mattermost/mattermost#11054
https://mattermost.atlassian.net/browse/MM-15943

Related Pull Requests

mattermost/mattermost-plugin-jira#250

@janvt
Copy link
Contributor Author

janvt commented Jul 26, 2019

It seems I commited some changes to package-lock.json... Correct? Incorrect? Let me know what I should do.

@jasonblais jasonblais added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jul 26, 2019
@jasonblais jasonblais added this to the v5.16.0 milestone Jul 26, 2019
@aaronrothschild aaronrothschild removed the 1: PM Review Requires review by a product manager label Jul 26, 2019
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Thanks again @janvt !

Not sure about package-lock.json.

@enahum do we commit changes to package-lock if nothing in the package.json changed?

@cpoile cpoile added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Jul 27, 2019
@cpoile cpoile self-requested a review July 27, 2019 18:28
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Just waiting for @enahum's feedback on whether to include the package-lock change. Don't want to merge by accident.

@cpoile cpoile added 2: Dev Review Requires review by a core commiter and removed 4: Reviews Complete All reviewers have approved the pull request labels Jul 27, 2019
@enahum
Copy link
Contributor

enahum commented Jul 27, 2019

If no changes to the package.json file there should not be changes to the lock file

@cpoile
Copy link
Member

cpoile commented Jul 28, 2019

@janvt Can you remove the changes to the package-lock? Thanks @enahum

@janvt
Copy link
Contributor Author

janvt commented Jul 29, 2019

Removed unnecessary changes to package-lock.json.

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 @janvt! Done UI check and looks good. Left comment on unit test.

@@ -315,6 +315,12 @@ export default class DotMenu extends Component {
text={Utils.localizeMessage('post_info.edit', 'Edit')}
onClick={this.handleEditMenuItemActivated}
/>
{pluginItems.length > 0 &&
<li
Copy link
Member

Choose a reason for hiding this comment

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

Please add id for testing, like id='menuItemDivider'

components/dot_menu/dot_menu_plugins.test.jsx Outdated Show resolved Hide resolved
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Thanks @janvt ! Approved on dev side (still awaiting the changes requested from Saturnino).

@cpoile cpoile removed the 2: Dev Review Requires review by a core commiter label Aug 7, 2019
expect(wrapper.find('#divider_post_post_post_id_1').exists()).toBe(false);

wrapper.setProps({pluginMenuItems: [{id: 'test_plugin_menu_item_1', text: 'woof'}]});
expect(wrapper.find('#divider_post_post_post_id_1').length).toBe(1);
Copy link
Member

Choose a reason for hiding this comment

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

This is failing on test. Element ID should be divider_post_post_id_1. Also please update to master.

Let me know once updated so we can merge. Thanks!

@saturninoabril saturninoabril added the Awaiting Submitter Action Blocked on the author label Aug 14, 2019
@saturninoabril
Copy link
Member

@janvt Let me know if you need help. I can update the test if you don't mind so we can merge it.

@janvt
Copy link
Contributor Author

janvt commented Aug 22, 2019

@saturninoabril yeah, that would be awesome. I've taken 2 runs at it, but not sure what I'm not understanding... :D

@saturninoabril
Copy link
Member

@janvt No worries, now updated - f3b1fcc

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 @janvt for your contribution!

@saturninoabril saturninoabril merged commit e33ab57 into mattermost:master Aug 22, 2019
@saturninoabril saturninoabril added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Awaiting Submitter Action Blocked on the author labels Aug 22, 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
* MM-15943 render divider before plugin menu items

* revert changes to package-lock.json

* adjust dot menu unit test, remove dedicated plugin divider test

* fix test as suggested
skheria pushed a commit to uber-archive/mattermost-webapp that referenced this pull request Oct 3, 2019
* MM-15943 render divider before plugin menu items

* revert changes to package-lock.json

* adjust dot menu unit test, remove dedicated plugin divider test

* fix test as suggested
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
Projects
None yet
8 participants