pnp / sp-dev-fx-property-controls

Reusable SPFx property pane controls - Open source initiative
https://pnp.github.io/sp-dev-fx-property-controls/
MIT License
236 stars 151 forks source link

Any interest in a File Picker property control? #161

Closed hugoabernier closed 4 years ago

hugoabernier commented 5 years ago

Category

Expected / Desired Behavior / Question

I recently created a file picker control for a sp-dev-fx-webparts sample. It was designed to be almost identical to the out-of-the-box file picker, with support for OneDrive, sites, upload, link, recent, and web search features. The sample itself (the "comparer" component) was almost a throwaway piece -- an excuse to have multiple file pickers in one property pane.

It doesn't need to be granted any API permissions (but it does need a Bing API key if you want to use the Search function).

The file picker in the screen shot below is 100% custom code -- no out-of-the-box file pickers were used.

demo of control

I wrote it with the intent to share it as a PnP property control. It is designed to support both images and files, but I have only tested it with images right now.

I also briefly considered making it extensible by allowing devs to add their own tabs, but felt it was more important to deliver a file picker experience that users would feel familiar and decided against it. I'm open to the idea, if anyone has a valid usage scenario.

You can find the sample in the sp-dev-fx-webparts repo.

Is this something that the community would use? I would need help testing it in various scenarios (e.g.: no OneDrive, RTL, non-Windows OS, etc.), and I would need to make sure that the document support works, but I'd be happy to share.

AJIXuMuK commented 5 years ago

I’m definitely interested! =)

estruyf commented 5 years ago

@hugoabernier definitely interesten in it, but I would recommend to not invest too much time in it. This is on my list to investigate how we can leverage what the first party controls are using.

Just to give a bit more background information. In the initial release of the controls, the control was supposed to be included, but we never received the permission to use the APIs.

ATM Microsoft is also creating a file browser: https://www.npmjs.com/package/@microsoft/file-browser - So will do my best to check out how we could leverage what’s already built in SharePoint, instead of creating this one from scratch. Although I really like what you already built, but if we could leverage of what is already built, it would make maintainability a lot easier.

hugoabernier commented 5 years ago

@estruyf the control is already written, so I'm not planning on spending more time on it unless we plan on adding it as a control.

To be honest, I would prefer it if there was a first party version.

Let me know what you find. In the meantime, those who need such a control can always use the code from my repository.

estruyf commented 5 years ago

@hugoabernier feel free to add this as a property pane control. Discussed it internally, and it might be better that we keep ownership of the code. So having our own control, simplifies this process.

hugoabernier commented 5 years ago

@estruyf it would be my pleasure. I'll get to it as soon as I submit my PR for the updated Richtext control with Rooster.

AJIXuMuK commented 5 years ago

Hi @hugoabernier! I looked at the web part example and it looks great! Really cool thing to add to the controls as well.

Here are some thoughts I want to share with you:

  1. I'm pretty sure you are on the same page, but it would be great to have multiselect available
  2. OneDriveTab and SiteFilePickerTab look pretty similar. Maybe it would make sense to create some shared control like FileExplorer and use it in both tabs? In that case you could also make OneDriveServices more generic to work both with SharePoint site and OneDrive site.
  3. As currently we're waiting for SPFx in Teams release, it also make sense to somehow add selection from Channel folder
  4. From code perspective I would change event handlers from {() => { this._someMethod(); }} constructions to {this._someMethod.bind(this)}... And maybe also lead all methods declarations to a common definition. Right now for public methods you're using public method(): type {}, for private: private method = (): type => {}.
hugoabernier commented 5 years ago

@AJIXuMuK thanks for looking at the code.

Your feedback is all very valuable! You really did look at the code!

Here are my responses:

  1. Yes, the plan is to allow multiselect, but I didn't spend any cycles to handle it. Definitely in the plans.
  2. My primary goal when designing the control was to be as close as possible to the OOB file picker. I kept the two tabs distinct because the OOB version seem to be two different components, but I also think there is an opportunity to combine both OneDriveTab and SiteFilePickerTab -- but it may be at the loss of a little fidelity with the OOB control.
  3. Great idea. Plan was to make the tabs expendable to allow developers to add more tabs (I'd love a CameraTab for mobile devices to add a picture directly from a camera), but the ChannelFilePickerTab seems like a must have (and higher priority ).
  4. Still new to TS + React. I noticed Office UI Fabric has replaced all their .bind() calls with {()=> { this._someMethod(); } and I was following their style, but didn't really undestand why :-). I'll definitely research this further.

Please continue providing feedback. I'm putting finishing touches on the RichText control, then I will be switching my efforts back to this FilePicker control.

AJIXuMuK commented 5 years ago

@hugoabernier honestly, I haven't read the Office UI Fabric guides :) I was more thinking of code consistency. But yes, if you find information that this style is beneficial then why not :) My thoughts are just subjective opinion )

AJIXuMuK commented 5 years ago

Hi @hugoabernier! While working with the control, I've found one issue: the file picker (OneDrive Tab) will fail if there are more than one page to display. As example, you have 10 folders in OneDrive, but current rect can contain 7 only. In that case _onRenderPage will fail as it assumes that items property is always defined. But in the situation I've described items === undefined for the "spacer".

Please, let me know if you need any other details.

UPDATE For the fix it's just enough to add:

 if (!items) {
            return _defaultRender(pageProps);
        }

to the _onRenderPage

hugoabernier commented 5 years ago

Thanks @AJIXuMuK , great catch! I'll make sure to add your fix in my code.

AJIXuMuK commented 5 years ago

One more thing: in _getListItems you have this code:

const thumbnailUrlTemplate: string = listDataStream.ListSchema[".thumbnailUrl"]
        .replace("{.mediaBaseUrl}", listDataStream.ListSchema[".mediaBaseUrl"])
        .replace("{.callerStack}", listDataStream.ListSchema[".callerStack"])
        .replace("{.driveAccessToken}", "encodeFailures=1&ctag={.ctag}");

Probably, in the last line listDataStream.ListSchema[".driveAccesToken"] has been lost, and thumbnails are not displayed

WayneJSawyer commented 5 years ago

Looking forward to this landing in the Property Controls, as a much needed modern replacement to the old AssetUrlSelector. Thanks in advance @hugoabernier

AJIXuMuK commented 4 years ago

FilePicker has been already included in the library