Skip to content
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

Closed

Conversation

joachimeichborn
Copy link
Contributor

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.

…listgit

Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
…es as active and breadcrumbs

Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
@korelstar
Copy link
Member

Very nice! Could you please provide some screenshots? That would make reviewing easier for the design people.

@joachimeichborn
Copy link
Contributor Author

joachimeichborn commented Apr 5, 2022

Sure, here are two screenshots. The first one shows the desktop view. The first column contains the category selection, the second one the note of the selected category. The third column contains the note itself and the fourth is the sidebar (that can be shown or hidden as before):
grafik

The second screenshots illustrates how it looks on smaller screens (e.g. mobiles). The category selection can be shown as overlay on the left as before. The notes selection is always visible but can be switched between full width and narrow mode (in the screenshot the narrow mode is active). The details sidebar works as usual.
grafik

@korelstar
Copy link
Member

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:

  • Why didn't you use the standard component AppContentList for the notes list?
  • Why did you chose to always show the notes list in mobile mode (even on very narrow screens)? I think it would be better to hide the notes list by default in order to provide more space for editing the current note. This is how other apps behave and I think it's the behavior of the component AppContentList.
  • Users cannot change the width of the notes list. I think changing the content list's width is also default behavior of the component AppContentList and it is very helpful in many cases.
  • You've introduced a new breadcrumb navigation. However, it has no function. Is this intended or do you have a plan to add functionality? I like the design of the breadcrumb navigation, but I'm unsure if we really need this. It's just another widget that will reduce the space for editing notes. When looking at Feature/788/toolbar #790, this becomes even more important. But I would like to hear some other opinions @nextcloud/notes @nextcloud/designers

@marcoambrosini
Copy link
Member

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 <joachimeichborn@users.noreply.github.com>
…tton

Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
@joachimeichborn
Copy link
Contributor Author

@korelstar Thank you for your feedback. I just wasn't aware off the possibilities that AppContentList provides, I have created a new version that uses it. That improved usability, especially on mobile devices.
The breadcrumb navigation currently is not interactive and only shows the category information. I have some ideas how it could provide interactive features in the future, but that is a task on its own. I have disabled it on mobile devices as it conflicts with the overlay to switch between list and details view (and that's a pity as it is especially valuable on mobile devices where the details view does not provide any context to the displayed note).

@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...

@newhinton
Copy link
Contributor

newhinton commented May 28, 2022

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

@korelstar korelstar added this to the 4.5.0 milestone May 29, 2022
@SamuXzX
Copy link

SamuXzX commented Jul 13, 2022

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:
Untitled1

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:
178241898-a932f1ca-f976-4074-aeca-a2e26213f673


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:

The note/category-view should now behave like browsing the filestructure of a filesystem: Show categories in the current scope, and notes below them. If a category is clicked, replace the current scope to the new one, and repeat.


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.

@newhinton
Copy link
Contributor

@SamuXzX This is a little bit off-topic here, this PR is specifically for the tree column layout for the current version. Any changes to categories should probably be discussed in #413

@SamuXzX
Copy link

SamuXzX commented Jul 13, 2022

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 <joachimeichborn@users.noreply.github.com>
@joachimeichborn
Copy link
Contributor Author

@newhinton I missed you comments, so let me answer them now:

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!

This is done to achieve consisteny with the mail app. I would also prefer more spacing, but I guess consistency is more important here.

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

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.

Also i think it would be good if we would add a new element "Favorites" in the leftmost bar

Agreed, but not something that should be done as part of this pull request

@joachimeichborn
Copy link
Contributor Author

@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?

@korelstar korelstar modified the milestones: 4.6.0, 4.7.0 Oct 1, 2022
@newhinton
Copy link
Contributor

@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.

@korelstar
Copy link
Member

korelstar commented Oct 14, 2022

@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?

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.

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.

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:

  1. Publish new release for Nextcloud 25 release version 4.6.0 #929 (with update vue library to final version), since this is time-critical (Nextcloud 25 will be released on 2022-10-18).
  2. Finalize and merge in-app-wiki implement in-app-wiki #862, since it is nearly finished.
  3. Finalize and merge three column layout Three column layout #842, since it is highly demanded and there will be the possibility for further optimizations after that.
  4. Finalize and merge toolbar Feature/788/toolbar #790, here are still many things to be done, so I would like to move it to the end.

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.

@newhinton
Copy link
Contributor

@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 <joachimeichborn@users.noreply.github.com>
Signed-off-by: Joachim von Eichborn <joachimeichborn@users.noreply.github.com>
@joachimeichborn
Copy link
Contributor Author

@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.
@korelstar Is this PR on track for 4.7.0 now or is something missing?

@newhinton
Copy link
Contributor

@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)?

@korelstar
Copy link
Member

Thanks for updating this PR. The maintenance of this app has been transferred to the Nextcloud team in the meanwhile.
@juliushaertl could you please take over?

@newhinton
Copy link
Contributor

@korelstar
Oh that is sad to hear, are you leaving temporarily or is it a permanent thing?

I would have liked to discuss some changes i'd like to make, but i guess i'll talk to @juliushaertl

@juliushaertl
Copy link
Member

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. :)

@juliushaertl juliushaertl added the enhancement New feature or request label Feb 21, 2023
@juliushaertl juliushaertl mentioned this pull request Feb 21, 2023
22 tasks
@juliushaertl
Copy link
Member

juliushaertl commented Feb 24, 2023

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:

  • More spacing around the icon in the list
  • Entries need margin to left and right so they don’t touch the navigation on the left and the border on the right
  • Adjust style of the date headings
    • More spacing to the top
    • Make sure it uses same component as e.g. in the Talk search for category dividers
  • Breadcrumbs bar is not needed, the category list in the navigation is enough

Later enhancements

  • Have same action menu for notes in the list view as in the notes detail view
  • 3-dot menu is primary style, should be standard icon-only
  • Between "All notes" and "Uncategorized" there could be a "Favorites" category like in the Android app
  • Between "Uncategorized" and the category list there should be a header "Categories", like in Contacts
  • Also like in Contacts, there could be a "+" to quickly add categories directly there
    • Then later we can talk about a way to quickly remove categories via there. Currently the category is removed if there is no note in it. 2 options for deletion could be to 1) Delete category only, notes will be uncategorized then or 2) Delete category including the notes in it (like a folder deletion).
  • Within categories, sorting should also just be by most recently edited (within subcategories as well)

@newhinton
Copy link
Contributor

Also like in Contacts, there could be a "+" to quickly add categories directly there

I dont think that is possible. Afaik categories are "computed", meaning they only show up if a category contains notes.
An added category does not have a note, and therefore does not exist.

This also applies to deletion, but that is easier to solve.

But doesn't this belong here: #413?

@juliushaertl
Copy link
Member

@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.

@joachimeichborn
Copy link
Contributor Author

@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!

@juliushaertl
Copy link
Member

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 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants