-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Quick-Access to favorite folder in left sidepanel in files-app #9720
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9720 +/- ##
============================================
- Coverage 51.72% 2.04% -49.68%
- Complexity 25733 26007 +274
============================================
Files 1636 1585 -51
Lines 96030 88848 -7182
Branches 1384 0 -1384
============================================
- Hits 49667 1815 -47852
- Misses 46363 87033 +40670
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Thanks for your pull request and welcome! :) 🎉
I added quite a few comments to explain some of the required changes :)
Feel free to ask for more details if I did not explained properly ;)
<?php } ?> | ||
|
||
|
||
<?php if($_['favoritesFolders']>0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be created. All navigation elements should go into the navigationItems. Otherwise this goes against the simple implementation we did here. We should not add multiple types of navigations items.
This will also break the pinned system if another app decide to add one new in the nav item list.
If you did that just to add your spacer, please create a new condition in the main loop and a check to detect the use of a spacer item :)
apps/files/css/files.scss
Outdated
@@ -106,6 +106,19 @@ | |||
background-image: url('../img/delete.svg?v=1'); | |||
} | |||
|
|||
.nav-sidebar-spacer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be standardise an put into the apps.scss
file.
- Please cut the code of the settings-caption class and put it into the
apps.scss
file (change the class name to something likeapp-navigation-caption
- Update the uses of the old
settings-caption
across the server https://github.com/nextcloud/server/search?utf8=✓&q=settings-caption to you new class name - use the same class for your
Favorites
navigation element
Like so any other app that also register a spacer within the files app (from anywhere like their own application or from another nextcloud app) won't have anything to edit in the server core code :)
|
||
$helper= new \OCA\Files\Activity\Helper($tagger); | ||
$favElements = $helper->getFavoriteFilePaths($this->userSession->getUser()->getUID()); | ||
$favItems = $favElements["items"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single quotes please ;)
$favElements = $helper->getFavoriteFilePaths($this->userSession->getUser()->getUID()); | ||
$favItems = $favElements["items"]; | ||
|
||
$key="show_Quick_Access"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same :p
} | ||
|
||
$i=0; | ||
foreach($favElements["folders"] as $elem){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here also
if($showFavoriteQuickAccess){ | ||
$nav->assign('favoritesItems', $favItems); | ||
$nav->assign('favoritesFolders', $favFolder); | ||
$nav->assign('setQuickAccessChecked', "checked"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weee 🙈
<?php $pinned = 0 ?> | ||
<?php | ||
$pinned = 0; | ||
$trashelement = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment, we don't need to create specific entries for the trashbin, this should stay clean.
} ?>"> | ||
<a href="#" class="icon-quota svg"> | ||
<a href="#" class="nav-icon-quota svg"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the nav-icon-quota
class exists? 🤔
<label for="webdavurl"><?php p($l->t('WebDAV'));?></label> | ||
<input id="webdavurl" type="text" readonly="readonly" value="<?php p($_['webdavurl']); ?>" /> | ||
<input id="webdavurl" type="text" readonly="readonly" value="<?php p(\OCP\Util::linkToRemote('webdav')); ?>" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? Was it not working? (I did not checked) :)
@skjnldsv The quotes and the webdav/quota-changes are fixed, they were made by mistake, thanks for pointing out! The other changes will need some time since they are not quick fixes. |
Sorry, I should have been more clear :) server/settings/css/settings.scss Lines 1244 to 1257 in 760b28b
Therefore, since your pull request will most likely be integrated into 14, every app that want to add a caption-like design into their navigation will be able to use our design thanks to you 😉 |
It shouldn't be that much, don't worry. Since we already have the code to add items to the app navigation, just make your script add the favorites items with the You may need to edit the |
core/css/apps.scss
Outdated
@@ -561,6 +561,25 @@ kbd { | |||
.app-navigation-entry-menu ul { | |||
list-style-type: none; | |||
} | |||
|
|||
.settings-caption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app-navigation-caption
would be better please :)
Yeah, you are right about the css-style-name, forgot to change its name after getting it to work :D But there is still a last todo on my list, the Checkbutton wich enables this functionality for the user is not properly set on page load. It works and sets the correct value in the backend, but the current state is not applied on page load. Where do i need to set this value initially? |
@@ -40,9 +42,14 @@ class="nav-icon-<?php p($item['icon'] !== '' ? $item['icon'] : $item['id']) ?> s | |||
<input class="checkbox" id="showhiddenfilesToggle" checked="checked" type="checkbox"> | |||
<label for="showhiddenfilesToggle"><?php p($l->t('Show hidden files')); ?></label> | |||
</div> | |||
<div id="files-setting-showFavoriteQuickAccess"> | |||
<input class="checkbox" id="showfavoritequickaccessToggle" checked="checked" type="checkbox"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there is still a last todo on my list, the Checkbutton wich enables this functionality for the user is not properly set on page load. It works and sets the correct value in the backend, but the current state is not applied on page load. Where do i need to set this value initially?
The status is always checked by default here.
Either you set the state on javascript on load (not quite fond of this), or you pass it via php using the getUserValue
config check (Better solution I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great by the way! 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the checked="checked" does not change the state. It is always disabled by default, and i think it is set by javascript to unchecked. I have tried to mimic the behaviour of the showHiddenFiles-Toggle, but it does not seem to work. I don't now enough about the internal workings of the core, so i'm stuck here
And thank you for the compliment ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check the source code to see if php is properly setting this up.
But like Jan said, don't bother with the settings, remove it, it will be easier :)
And it's a great addition, so I guess less work from you and more pleasure for the users ;)
Good stuff! See my design comments in the issue at #9714 :) |
core/css/apps.scss
Outdated
pointer-events:none; | ||
} | ||
|
||
.app-navigation-subelement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I get it now! :)
There is already a standard for this:
Look at our documentation : https://docs.nextcloud.com/server/13/developer_manual/design/navigation.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work! Let's review this :)
- The Favourite first item menu is not collapsible, this should be possible
- The three-dot-more-icon should be hidden until the user have at least one directory favourited
- The list is not updated in real time
That's all I found so far :)
Thanks a lot!
What do you mean with "The Favourite first item menu"? |
I mean the top level menu. The entry that says "Favorites" :) |
@skjnldsv Where do i find the method which marks a file as a favorite? To make this thing "responsive", i need to add a function there, which adds or removes the item from the list. I cant seem to find this method, any hint? |
@newhinton Sure :) server/apps/files/js/tagsplugin.js Lines 89 to 160 in f347e2e
|
apps/files/js/navigation.js
Outdated
var dotmenu = document.getElementById("quickaccess-list"); | ||
dotmenu.style.display=''; | ||
|
||
if(!$("#favorites-toggle" ).hasClass('open')){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toggleClass
https://api.jquery.com/toggleClass/
@skjnldsv check it out now, it should be pretty finished now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful! :)
The default would be dateadded, which would be the good first step you mentioned. We have already passed that after introducing sorting in any way. The difference in code to maintain is not really that big, counted ~65 Lines of code more in the custom order, which includes brackets and so on. Last Date modified will NOT always show relevant stuff, since a cloud is often a shared medium (shared by bots which sync stuff, friends moving files in your shared folders, etc.). This will often result in an order which is not understandable by the user. It is unclear why it changed, since there is no explanation how it sorts. We dont need to make it discoverable, if the user never detects it, he can just use last date added, which does not have the ability to change randomly. A user could drag a folder into another folder, which could have WAY more serious consequences (synced folders, dataleak in shared folders, etc). I do not consider accidential dragging of elements which pose no risk at all (except from a messed up sorting, which can be solved by dragging it back) And as i stated earlier, dragging on mobile is not possible. You simply cannot drag on mobile browsers, if you do not implement it specifically. You dont need a default, because it's a custom order. From a feature-based standpoint, i consider this feature ready. I have implemented what i intended to achieve here, which will add benefit to users if they decide to use it. I do not consider "lastdatemodified" as a good design choice, since it introduces intransparency and confusion to a less savy user. Also it limits the benefits since you would always need to find your folders again. As long as there are no technical problems, bugs or nessessary codestylechanges, i consider the work finished, and i will not change the sorting since then i would have developed a feature which i would never use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Adding a file to favorite create a new entry in the left menu with the folder icon despite the fact that the starred file is not a directory
- On the favorite menu, when opening a folder details sidebar, clicking the star keeps adding entries to the menu and do not remove the directory from the favourites
@jancborchardt It should be noted that my comment is only valid for lastdatemodified sorting. If you really think custom sorting is over the top (which i disagree), we could settle by sorting it alphabetically, which would not add a changing ui element. This would be the next sensible thing to do. @skjnldsv Thanks for pointing this out, i'll fix it! |
@jancborchardt What about a dropdown menu? I know, its still a setting more in the ui, its still a little bit more to maintain, but i'm trying to make it work for everyone. Also it is a more slender solution to the sorting-problem as the last time. Your thoughts about that? |
@skjnldsv I should have fixed the bugs |
@@ -17,7 +17,7 @@ class="pinned <?php p($pinned === 0 ? 'first-pinned ' : '') ?><?php | |||
<a href="#" class="icon-quota svg"> | |||
<p id="quotatext"><?php | |||
if ($_['quota'] !== \OCP\Files\FileInfo::SPACE_UNLIMITED) { | |||
p($l->t('%s of %s used', [$_['usage'], $_['total_space']])); | |||
p($l->t('%s of %s', [$_['usage'], $_['total_space']])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah no! Do it in another pr please :)
Otherwise it's confusing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small missunderstanding ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok from me.
Please rebase to master.
Only downside is that clicking a favourited folder change the whole page instead of loading the folder only. But we can fix this in a later pr.
Sigh... new pr is here: #10176 😞 |
Add Quick-Access to favorite folder in left sidepanel in files-app #9720
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
How can I pin an folder to the left sidebar? |
@DarkknightAK |
Cause now you have 5 predefined items in the sidebar and you need two clicks (favorites -> folder) you come to your favorite folders. For tags you need three clicks (Tags drop down tag search and than have the results to click) |
Ah okay that is not possible at the moment. The only way to add a folder to the sidebar is to mark it as a favorite however, you can expand the favorites in the sidebar, this will allow you to get to a folder quickly. it should stay expanded over sessions |
Ah cool that's fine to me. Thanks. would it be also possible to pin labels that way to have an expand at labels and have there the labels. |
Probably yes, but it needs to be developed and accepted. Also since this pr is already closed, could you open a new issue for that? |
I have implemented the feature request from Issue #9714.
Fixes #9714
The only thing which does not properly work, is the initial State of the checkbox which enables this functionality. It is properly set, but the initial state is always unchecked, regardless of the true value.