Skip to content

Conversation

@PVince81
Copy link
Contributor

Adds right sidebar with registrable panels (still WIP)

For #17655

Note: file actions are currently disabled for demo purposes. When clicking on a file it will show the sidebar but not perform any action.

  • TODO: rewire click events to have the default file action work on the left side of the row but not the right side
  • TODO: implement tabs logic
  • TODO: implement default tab
  • TODO: styling
  • TODO: sidebar must close when unselecting file or selecting multiple files ?

@PVince81
Copy link
Contributor Author

Basically this brings the following:

  • DetailsView which is the right sidebar / panel. On this class it's possible to register subviews
  • DetailFileInfoView is the base class for any file info block to be displayed in the top panel. Needs to be registered with detailView.addDetail(...)
  • MainFileInfoDetailView implements DetailFileInfoView to provide the basic info like thumbnail, file name
  • DetailTabView is the base class for info tabs. Needs to be registered with detailView.addTab(...) (the logic isn't implemented yet)
  • styles can be tweaked in detailsView.css

I know... a lot of classes and not so fancy names...

@PVince81 PVince81 added this to the 8.2-current milestone Jul 15, 2015
@PVince81
Copy link
Contributor Author

@PVince81
Copy link
Contributor Author

@jancborchardt I think it's still too early to tweak the styles. I'll let you know when it's ready 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work as there is a hardcoded value of 36x36 as a fallback and we need something larger. We keep running into this issue.
Besides, I don't think we need lazy loading here. Just point to the preview using generatePreviewUrland the browser will show it when ready.

Copy link
Contributor

Choose a reason for hiding this comment

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

The mockup indicates 50x50, but that's too small for the prevew of a selected file. Maybe we should load a 200x200 preview and let the browser resize it based on the available width, if the sidebar is responsive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it as todo. The FIXME already says that what I did here is wrong 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah, I only looked at the todos :D

@PVince81
Copy link
Contributor Author

  • TODO: load previews with the correct API, maybe with 200x200 format and reduce it to 50x50 ? ( @oparoz feel free to comment/rectify)

@PVince81
Copy link
Contributor Author

I realize that the file info details (the top view) cannot be extended block-wise.
Maybe the easiest is to simply pass the jQuery element to plugins so plugins can embed themselves wherever. Here you can see that "Owner: " is in its own block. That one is provided by the "files_sharing" app.

I'm not too happy yet about the way how to provide tab view and detail view, it seems quite cumbersome to implement/extend a big class just for a few lines.

Also due to the problematic loading order of JS files I had to do this: https://github.com/owncloud/core/pull/17656/files#diff-593f07f9e5d32f4d3fdebbfb2fa5b71aR144

@PVince81
Copy link
Contributor Author

  • hightlight current row
  • full height scrollbar on the sidebar (let the contents flow, no inner scrollbars)

@PVince81
Copy link
Contributor Author

@PVince81
Copy link
Contributor Author

  • TODO: unit tests for the panels logic

@PVince81
Copy link
Contributor Author

I'm not sure if we want to do the share dialog task for now.
Maybe we can first get this in a mergable state and the sidebar will initially have no tabs shown at all.
So we need to get the interaction with the file list right.

When testing, please make sure to test the sidebar in the other views: "Shared with you", "Deleted files", etc. 😄

@blizzz
Copy link
Contributor

blizzz commented Jul 15, 2015

Basically this brings the following:

  • DetailsView which is the right sidebar / panel. On this class it's possible to register subviews
  • DetailFileInfoView is the base class for any file info block to be displayed in the top panel. Needs to be registered with detailView.addDetail(...)
  • MainFileInfoDetailView implements DetailFileInfoView to provide the basic info like thumbnail, file name
  • DetailTabView is the base class for info tabs. Needs to be registered with detailView.addTab(...) (the logic isn't implemented yet)
  • styles can be tweaked in detailsView.css

I know... a lot of classes and not so fancy names...

Nothing against many classes, if they make the whole feature better readable, understandable and maintainable. Of course this can be done, but I do not see it here.

The naming however should make you roughly understand what this is it about. DetailsView is pretty general, more obvious would be to have Sidebar or Panel in it's name indeed, but would cost you flexibility to change it in the future to a e.g. bumping circle. As it shows details, I am good with the naming.

Most importantly, the structure makes sense to me. All question that occurred to me were well answered in this approach.

@PVince81
Copy link
Contributor Author

@blizzz thanks for the feedback 😄
I'd say let's keep this approach then and we can adjust later if needed.

@PVince81
Copy link
Contributor Author

  • fix scrolling: the top of the sidebar must always stay visible when scrolling down the file list (ex: when getting details of a file on page 12). Add a second scrollbar inside the sidebar to scroll its whole content.

@PVince81
Copy link
Contributor Author

right-sidebar

@PVince81
Copy link
Contributor Author

  • TEST: public page

@PVince81 PVince81 force-pushed the files-rightsidebar branch from 495d422 to 62d1388 Compare July 16, 2015 10:52
@PVince81
Copy link
Contributor Author

I changed the scrolling behavior: now the sidebar has its own scrollbar.
Unfortunately it doesn't look very good when there are two scrollbars, but there might not be any better way around, unless we add an inner scrollbar inside the tab content instead.

@jancborchardt I think you can start tweaking the styles now and test the interaction 😄
I left the debug tabs so you can test the scrolling.

right-sidebar2

@PVince81
Copy link
Contributor Author

  • TEST in internet explorer

@PVince81 PVince81 changed the title [WIP] Basic work for right sidebar Basic work for right sidebar Jul 16, 2015
@PVince81
Copy link
Contributor Author

  • BUG: fix display name in trash bin (use fileInfo.displayName)

@jancborchardt
Copy link
Member

@PVince81 please do not use jQuery UI here, as we want to ideally get rid of it. Simply use buttons, and I will style them properly similar to the database chooser on installation.

@PVince81
Copy link
Contributor Author

The reason I used tabs is just because I wanted to save time and saw a working example on the LDAP settings page.

  • TODO: implement own tabs plugin/logic

@blizzz blizzz force-pushed the files-rightsidebar branch from 1d30248 to d1439ec Compare August 6, 2015 23:24
@blizzz
Copy link
Contributor

blizzz commented Aug 6, 2015

My attempt to fix the test. Also rebased.

@ghost
Copy link

ghost commented Aug 7, 2015

🚀 Test PASSed.🚀
chuck

@scrutinizer-notifier
Copy link

A new inspection was created.

@ghost
Copy link

ghost commented Aug 7, 2015

💣 Test FAILed. 💣
nooo432

@PVince81
Copy link
Contributor Author

JS unit tests fail:

07:24:11    Expected '' to equal 'One.txt'.
07:24:11        at /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/apps/files/tests/js/mainfileinfodetailviewSpec.js:54
07:24:11    Expected '' to equal '123456789 bytes'.
07:24:11        at /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/apps/files/tests/js/mainfileinfodetailviewSpec.js:56
07:24:11    Expected '' to equal 'July 16, 2015 9:02 PM'.
07:24:11        at /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/apps/files/tests/js/mainfileinfodetailviewSpec.js:58

I'll look into this.

@PVince81
Copy link
Contributor Author

I hereby approve all commits from @blizzz and @Henni and @jancborchardt with this 👍

Please review my commits too so we can get this merged.

@blizzz
Copy link
Contributor

blizzz commented Aug 10, 2015

And so am I honoured to give a 👍 to all commits from @PVince81 @Henni and @jancborchardt 🎆

@PVince81
Copy link
Contributor Author

Unless there is any objection, I will merge this as soon as Jenkins gives the green light. 💡

@ghost
Copy link

ghost commented Aug 10, 2015

🚀 Test PASSed.🚀
chuck

PVince81 pushed a commit that referenced this pull request Aug 10, 2015
@PVince81 PVince81 merged commit 15e16d3 into master Aug 10, 2015
@PVince81
Copy link
Contributor Author

Here we go.

Please raise tickets separately.

I'll soon make a ticket for the file actions (favorites + click on thumbnail)

@PVince81
Copy link
Contributor Author

Raised ticket to implement the missing actions (thumbnail click + favorite click): #18169

@jancborchardt
Copy link
Member

@PVince81 great work man! :)

@DeepDiver1975
Copy link
Member

@PVince81 branch can be deleted?

@MorrisJobke MorrisJobke deleted the files-rightsidebar branch August 10, 2015 20:56
@PVince81
Copy link
Contributor Author

branch can be deleted?

Yes. Strangely Github didn't show me the button yesterday.

@MorrisJobke
Copy link
Contributor

Yes. Strangely Github didn't show me the button yesterday.

On mobile? Because on mobile it isn't present

@PVince81
Copy link
Contributor Author

No, on desktop web UI. It was probably a temporary glitch.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants