-
Notifications
You must be signed in to change notification settings - Fork 136
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
Three column layout #842
Three column layout #842
Conversation
…listgit Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
…es as active and breadcrumbs Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
f9619f3
to
0eb4605
Compare
Signed-off-by: Joachim von Eichborn <[email protected]>
Very nice! Could you please provide some screenshots? That would make reviewing easier for the design people. |
Hey @joachimeichborn ! I'm so sorry, that I haven't found earlier some time for looking into this. Your work looks really good! I have some questions/remarks about it:
|
Nice, one additional comment from my side would be to move the assignment of category functionality to a little dialog window instead of deploying the sidebar for it. This way we'd avoid this rather unpleasant 4 column view |
Signed-off-by: Joachim von Eichborn <[email protected]>
…tton Signed-off-by: Joachim von Eichborn <[email protected]>
@korelstar Thank you for your feedback. I just wasn't aware off the possibilities that @marcoambrosini I agree that the current solution to categorize notes could be improved, it is also quite hidden in the sidebar and require a lot of clicks (drag and drop from the notes list onto a category might be an option, at least on large screens). However, this is a different story that should be tackled independently, the change proposed in this PR is already big enough... |
Just some small suggestions i found while looking at your otherwise awesome PR: The highlight of the current note in the new column has no left and right spacing, it looks a bit cramped at the sides. Also the burger menu for the navigation bar has no spacing at all and overlaps with notes from the column it is in. This was already the case before but now it heavily overlaps. Maybe a bit more space would look good! I really like that you colored the threedot menu on the right side Regarding the breadcrumb-bar, wouldn't it be an easy solution to map the elements to the selected category? eg. show all notes in the new bar from "Cat 1" if that element was clicked? Otherwise i think you should disable the "click" styling it currently has, because it implies that it can be clicked while not having functionality Also i think it would be good if we would add a new element "Favorites" in the leftmost bar |
In #879 I suggested a way to handle multiple levels of folders in to simplify the navigation and reduce unnecessary loadings. I suggested it with the current layout in mind, using the way the android app handles sub-categories as an example: The left column could be used to navigate the filesystem with a tree-style structure, which shows sub-folders only when clickign on their parent and hides them when they lose focus: The only problem with the tree-style structure could be that the list of folders could become very long. Maybe we could go for displaying only the the sub-folders under the current "scope" (parent folder) and keeping its name at the top with a button to go back off one level. I quote the lines of @newhinton I find useful from #413, where he/she sug:
Given the facts that we can discuss it here and that this thread is already related to #413, if you agree I could close the thread (#879) I opened. |
I thought that this thread could be related to the internal layout of the left column. If such internal layout is discussed in #413 then I'll delete my comments and answer in it instead. |
Signed-off-by: Joachim von Eichborn <[email protected]>
@newhinton I missed you comments, so let me answer them now:
This is done to achieve consisteny with the mail app. I would also prefer more spacing, but I guess consistency is more important here.
Unfortunately adding the desired behaviour is not as easy as one would expect as the categories are not handled by router so far. So I would go with disabling the "click" style but I found no way to do it - it seems the Breadcrumb component does not support that. So wht options at hand is having it as it is now or removing it. I prefer having it, but removing it is a simple change.
Agreed, but not something that should be done as part of this pull request |
@korelstar what is the reason that this pull request did not make it into milestone 4.5.0 as planned? Is there something missing before it can be integrated? |
@joachimeichborn I dont know the reasons why it didnt make into 4.5, but i guess you will have to update your branch since notes got a rather big update for nextcloud 25. However, before you do we should probably time this properly, because i will also have to update my PR's. I am nearly done with the help(#862) but the toolbar (#790) needs to be rebased. It would be easier if we would do it in sequence, maybe @korelstar can coordinate this so that we dont have to rebase multiple times. |
I'm very sorry for my late reply. I had this PR on "WAIT" since I was waiting for some reaction from @nextcloud/designers regarding the breadcrumb navigation. The other reason is that I have too less free time for this project. I'm very sorry.
Yes, I think this is a good idea. Let's do the further process strongly sequential. I would like to do the next PRs in the following sequence:
In general, let's try to keep all PRs as small as possible in order to make reviewing simple. Therefore, I suggest to remove the breadcrumb from this PR and move it later into a separate PR. but let's have a look on this after #862 is merged. |
@joachimeichborn Do you know if you can get to this soonTM? If you like, i can take a shot at updating this branch so that we can move this along! |
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Joachim von Eichborn <[email protected]>
@newhinton I have merged the upstream master in my code and hope I got everything correct although it was sometimes a bit nasty to merge. For now I have made the breadcrumbs appear as non-interactive labels. But if this is still considered to be missleading, they may be removed as well. |
@joachimeichborn This is great! Thanks! Imho, the breadcrumbs are fine. Maybe you can give your opinion on the future use of your layoutchanges here (#413)? |
Thanks for updating this PR. The maintenance of this app has been transferred to the Nextcloud team in the meanwhile. |
@korelstar I would have liked to discuss some changes i'd like to make, but i guess i'll talk to @juliushaertl |
I'll have a closer look at this PR next week. @joachimeichborn Maybe you could put some up to date screenshots in here to ease getting some design input from @nimishavijay and @jancborchardt. :) |
We had a closer look at this during our design review call and this would be the current feedback UI wise in regards to the 3 column layout: In general this looks super nice and we'd love to get this merged as soon as possible. I've therefore split the feedback into things that I'd consider blockers for the merge and things that could also be done later on:
Later enhancements
|
I dont think that is possible. Afaik categories are "computed", meaning they only show up if a category contains notes. This also applies to deletion, but that is easier to solve. But doesn't this belong here: #413? |
@joachimeichborn Thanks a again for this contribution. I took some time trying to rebase the branch to the latest master branch but had some troubles due to the merge commits causing quite heavy conflicts while the actual diff is quite small. I managed to easily resolve those through a squash+rebase but this means that we loose the individual commits. Would you still be up for pushing the development forward on this? It would be great to get this into the next release I think. My current commit state is in #1021 which is now based on the most recent master branch and has a few minor adjustments. Let me know what you think and if you would be up for further pushing this forward. You could of course hard reset your pull request branch to the commit from the rebased branch to take over the conflict resolution, then we could continue here and close the draft. |
@juliushaertl I am fine with loosing individual commits if this helps to get the change released. In fact, I am currently quite short on time, so it would be great if you could take care of finally getting this PR into the release. You have my blessing for whatever is necessary to achieve that! |
Thanks. Then I'll be glad to take over and move this forward in #1021 Thanks for the hard work on this already and for pushing for this change. This is highly appreciated ❤️ |
After pull request #839 was about a conversative approach to improve category usability without changing the UI too much I have learnt that there is indeed a desire to have a three column layout that improves usability. This is my proposal for such a layout that also has at least limited support for responsive design.