MM-10657 Adding ability for plugin to hook the file upload button. #1488

crspeller · 2018-07-27T23:18:02Z

This is experimental. I have not actually written a plugin capable of using this functionality for real. There may be additional hooks required for a properly functioning file upload plugin.

Mattermost demo plugin PR: mattermost/mattermost-plugin-demo#2

jasonblais

Anything in particular you'd like me to review?

Wondering if BrightScout would like to help try this since they were gonna build a plugin using this component for a customer

crspeller · 2018-07-30T17:12:17Z

@jasonblais I have uploaded the zoom demo plugin. Just check how the menu that pops up looks and feels.

hmhealey · 2018-07-30T16:27:18Z

components/file_upload/file_upload.jsx

@@ -55,7 +56,7 @@ const holders = defineMessages({

 const OVERLAY_TIMEOUT = 500;

-export default class FileUpload extends PureComponent {
+export default class FileUpload extends Component {


Why not use a PureComponent here any more?

Because it's not pure anymore. It has state.

PureComponents can still have state. They just only re-render when their state or props change like you'd implemented a shouldComponentUpdate method

hmhealey · 2018-07-30T16:30:42Z

components/file_upload/file_upload.jsx

+ const FileDropdownComponent = (props) => {
+ return (
+ <button
+ onClick={(e) => {


Could this just use onClick={props.onClick}? Most of the time that we use e.preventDefault is because the click handler was on an <a> tag which would append a number sign to the URL, but since this is a <button>, it might not be necessary

Sure. I was just copying what was there.

hmhealey · 2018-07-30T16:30:56Z

components/file_upload/file_upload.jsx

+ <li>
+ <a>
+ <i className='fa fa-files-o'/>
+ {'Your Computer'}


This should be localizable

jasonblais · 2018-07-30T17:45:26Z

@crspeller

Can we dynamically left-align the text based on icon size across the different plugins?
Propose having Your computer instead of Your Computer as the default file upload option.
I realize this is a bit out of scope from this webapp PR: But when uploading a file via something like GoogleDrive or OneDrive plugin, the upload flow will be controlled by that plugin, correct? Example: whether or not the GoogleDrive/OneDrive file is uploaded into the message drafts or directly as a message.

Look and feel seems okay otherwise. Verified that file upload icon continues to be hidden when file uploads are disabled, as expected.

@asaadmahmood any feedback? The attached is the same on desktop and mobile view; and on center channel and RHS. Note that the change does not apply to mobile apps.

jasonblais · 2018-07-30T17:47:58Z

@asaadmahmood I'm wondering if it's clear what Your computer is in this context? Or should we have something like this in the dropdown instead

crspeller · 2018-07-30T17:53:29Z

@jasonblais

@asaadmahmood Can you do that? Not sure how to accomplish that with css.
Done.
Correct. I left that up to the plugin.

asaadmahmood · 2018-07-30T18:16:13Z

@crspeller Sure, will fix the alignment issue.
@jasonblais For sure, we need a more representative icon. We have font-awesome icons we can use like: https://fontawesome.com/v4.7.0/icon/laptop

mattermod · 2018-07-30T18:44:11Z

Spinmint test server created at: http:https://i-044313ddc36aad70d.test.spinmint.com

Test Admin Account: Username: sysadmin | Password: sysadmin

Test User Account: Username: user-1 | Password: user-1

Instance ID: i-044313ddc36aad70d

crspeller · 2018-07-30T18:51:03Z

@jasonblais Spinmint is ready for re-review.

jasonblais

I have a question for Asaad, but DM'd him on pre-release. Doesn't block this PR.

Thanks Chris!

mattermod · 2018-07-30T18:56:59Z

Spinmint test server destroyed

jwilander

Code looks good, just one comment

jwilander · 2018-07-30T19:50:07Z

plugins/registry.js

 return dispatchPluginComponentAction('PostDropdownMenuItem', this.id, component);
 }

+ // Register a post menu list item by providing some text and an action function.


Copy/paste error

hmhealey

I commented above, but the FileUpload component should still be a PureComponent since "pure" in this context means that the component only re-renders if it's state and/or props change

…attermost#1488) * Adding ability for plugin to hook the file upload button. * Tweaks and localization. * Updating icons * Fix docs. * Move back to pure-component.

crspeller added the 1: PM Review Requires review by a product manager label Jul 27, 2018

crspeller requested review from jwilander, hmhealey and jasonblais July 27, 2018 23:18

crspeller mentioned this pull request Jul 27, 2018

MM-10657 Adding file upload button to demo plugin. mattermost/mattermost-plugin-demo#2

Merged

Adding ability for plugin to hook the file upload button.

0704ab4

crspeller force-pushed the mm-10657 branch from e332b30 to 0704ab4 Compare July 27, 2018 23:21

crspeller added the Setup Old Test Server Triggers the creation of a test server label Jul 27, 2018

jasonblais reviewed Jul 27, 2018

View reviewed changes

hmhealey suggested changes Jul 30, 2018

View reviewed changes

Tweaks and localization.

f9ef3b0

crspeller force-pushed the mm-10657 branch from 7efbd30 to f9ef3b0 Compare July 30, 2018 17:54

jasonblais requested a review from asaadmahmood July 30, 2018 17:54

Updating icons

3638d68

jasonblais removed the Setup Old Test Server Triggers the creation of a test server label Jul 30, 2018

mattermost deleted a comment from mattermod Jul 30, 2018

crspeller added the Setup Old Test Server Triggers the creation of a test server label Jul 30, 2018

jasonblais approved these changes Jul 30, 2018

View reviewed changes

jasonblais removed the request for review from asaadmahmood July 30, 2018 18:56

jasonblais added 2: Dev Review Requires review by a core commiter and removed 1: PM Review Requires review by a product manager labels Jul 30, 2018

jasonblais removed the Setup Old Test Server Triggers the creation of a test server label Jul 30, 2018

mattermost deleted a comment from mattermod Jul 30, 2018

jwilander reviewed Jul 30, 2018

View reviewed changes

jwilander self-requested a review July 30, 2018 19:50

Fix docs.

089687f

jwilander approved these changes Jul 31, 2018

View reviewed changes

hmhealey suggested changes Jul 31, 2018

View reviewed changes

Move back to pure-component.

3bd4a93

hmhealey approved these changes Jul 31, 2018

View reviewed changes

hmhealey removed the 2: Dev Review Requires review by a core commiter label Jul 31, 2018

hmhealey merged commit ceb0ca0 into master Jul 31, 2018

hmhealey deleted the mm-10657 branch July 31, 2018 17:13

lindalumitchell added the Tests/Not Needed Does not require new release tests label Aug 7, 2018

amyblais added Changelog/Done Required changelog entry has been written Docs/Done Required documentation has been written labels Aug 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-10657 Adding ability for plugin to hook the file upload button. #1488

MM-10657 Adding ability for plugin to hook the file upload button. #1488

crspeller commented Jul 27, 2018

jasonblais left a comment

crspeller commented Jul 30, 2018

hmhealey Jul 30, 2018

crspeller Jul 30, 2018

hmhealey Jul 31, 2018

hmhealey Jul 30, 2018

crspeller Jul 30, 2018

hmhealey Jul 30, 2018

jasonblais commented Jul 30, 2018

jasonblais commented Jul 30, 2018

crspeller commented Jul 30, 2018

asaadmahmood commented Jul 30, 2018

mattermod commented Jul 30, 2018

crspeller commented Jul 30, 2018

jasonblais left a comment

mattermod commented Jul 30, 2018

jwilander left a comment

jwilander Jul 30, 2018

hmhealey left a comment

MM-10657 Adding ability for plugin to hook the file upload button. #1488

MM-10657 Adding ability for plugin to hook the file upload button. #1488

Conversation

crspeller commented Jul 27, 2018

jasonblais left a comment

Choose a reason for hiding this comment

crspeller commented Jul 30, 2018

hmhealey Jul 30, 2018

Choose a reason for hiding this comment

crspeller Jul 30, 2018

Choose a reason for hiding this comment

hmhealey Jul 31, 2018

Choose a reason for hiding this comment

hmhealey Jul 30, 2018

Choose a reason for hiding this comment

crspeller Jul 30, 2018

Choose a reason for hiding this comment

hmhealey Jul 30, 2018

Choose a reason for hiding this comment

jasonblais commented Jul 30, 2018

jasonblais commented Jul 30, 2018

crspeller commented Jul 30, 2018

asaadmahmood commented Jul 30, 2018

mattermod commented Jul 30, 2018

crspeller commented Jul 30, 2018

jasonblais left a comment

Choose a reason for hiding this comment

mattermod commented Jul 30, 2018

jwilander left a comment

Choose a reason for hiding this comment

jwilander Jul 30, 2018

Choose a reason for hiding this comment

hmhealey left a comment

Choose a reason for hiding this comment