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

Show file details within the tray dialog, rather than in a separate dialog #5139

Merged
merged 5 commits into from Dec 9, 2022

Conversation

claucambra
Copy link
Collaborator

Screen.Recording.2022-11-07.at.14.04.13.mov

Closes #4937

Please note that this PR depends on #5129

Signed-off-by: Claudio Cambra claudio.cambra@nextcloud.com

@jancborchardt
Copy link
Member

Nice work @claucambra! It does seem like it could use a back arrow button in the top left for example, or "Cancel"?

However, on "Back" it should still be saved, whereas on "Cancel" all changes should be rolled back, which might be a bit complex. What do you think @claucambra @nimishavijay?

@claucambra
Copy link
Collaborator Author

claucambra commented Nov 7, 2022

Nice work @claucambra! It does seem like it could use a back arrow button in the top left for example, or "Cancel"?

However, on "Back" it should still be saved, whereas on "Cancel" all changes should be rolled back, which might be a bit complex. What do you think @claucambra @nimishavijay?

I'm not sure what's being more broadly suggested here -- are we talking about the possibility of backward/forward navigation? Or just, at its core, just different positioning of the buttons?

If it's the latter I think that is overkill -- we are just switching back and forth between a one page hierarchy, where the user can just re-click the share button in the main activity list to return to the file details page, and just click the "done" button to remove the file details page.

If it's the latter, that's okay, but it clashes with our "done/accept/cancel" button positioning in the rest of the application, which is always at the bottom (see the user status modal popup, for example)

EDIT: also, what changes are you suggesting should be able to roll back? I don't think a "settings menu" type paradigm where the settings can be changed and then be applied or not applied is not a good fit here (and I have my gripes with it in general, it has no place for non-destructive changes)

src/gui/tray/Window.qml Outdated Show resolved Hide resolved
src/gui/tray/Window.qml Outdated Show resolved Hide resolved
src/gui/tray/Window.qml Show resolved Hide resolved
@jancborchardt
Copy link
Member

jancborchardt commented Nov 8, 2022

Notes from the design review call:

  • Only use ~90% of width, darken area outside on the left
  • Also make user status drawer only 90% height
  • Add x close button on top right instead of "Done" button
    • Also make left 10% darkened area clickable to go back
  • Replace share button with 3 dots button
  • Design updates for sharing actions: 🤝 Improvement to sharing design and flow server#26691
  • Right-click menu:
    • Section 1: Open locally, Open in web, Open parent folder
    • Section 2: drawer tabs: Activity, Sharing, Versions, …

src/gui/tray/Window.qml Outdated Show resolved Hide resolved
@claucambra claucambra force-pushed the feature/file-details-in-tray branch 2 times, most recently from b747138 to 1656b8f Compare November 9, 2022 12:11
@claucambra
Copy link
Collaborator Author

@jancborchardt @nimishavijay Here is the current state of the file details drawer:

Screen.Recording.2022-11-09.at.13.12.04.mov

@claucambra
Copy link
Collaborator Author

Notes from the design review call:

  • Only use ~90% of width, darken area outside on the left

  • Also make user status drawer only 90% height

  • Add x close button on top right instead of "Done" button

    • Also make left 10% darkened area clickable to go back
  • Replace share button with 3 dots button

  • Design updates for sharing actions: Improvement to sharing design and flow server#26691

  • Right-click menu:

    • Section 1: Open locally, Open in web, Open parent folder
    • Section 2: drawer tabs: Activity, Sharing, Versions, …

Bullets 2 and 6->9 we will do in a separate PR :)

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Very nice! :)

@karlitschek
Copy link
Member

Indeed very nice

Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

comments about some warnings coming from qml side
(see the inline comment)

there is also

qrc:/qml/src/gui/tray/Window.qml:190:30: QML FileDetailsPage: Binding loop detected for property "implicitHeight"

src/gui/tray/ActivityItemContent.qml Show resolved Hide resolved
@claucambra claucambra force-pushed the feature/file-details-in-tray branch 2 times, most recently from 04a3baa to 5de1c75 Compare November 14, 2022 13:50
@claucambra
Copy link
Collaborator Author

comments about some warnings coming from qml side (see the inline comment)

there is also

qrc:/qml/src/gui/tray/Window.qml:190:30: QML FileDetailsPage: Binding loop detected for property "implicitHeight"

Is fixed now :)

@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Merging #5139 (04a3baa) into master (6db9c95) will decrease coverage by 0.08%.
The diff coverage is n/a.

❗ Current head 04a3baa differs from pull request most recent head 5930fb4. Consider uploading reports for the commit 5930fb4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5139      +/-   ##
==========================================
- Coverage   57.63%   57.54%   -0.09%     
==========================================
  Files         138      138              
  Lines       17490    17399      -91     
==========================================
- Hits        10081    10013      -68     
+ Misses       7409     7386      -23     
Impacted Files Coverage Δ
src/common/syncfilestatus.h 50.00% <0.00%> (-50.00%) ⬇️
src/libsync/propagatorjobs.cpp 71.12% <0.00%> (-3.88%) ⬇️
src/libsync/syncfileitem.cpp 94.91% <0.00%> (-3.39%) ⬇️
src/libsync/discovery.h 57.14% <0.00%> (-2.12%) ⬇️
src/libsync/syncfileitem.h 39.65% <0.00%> (-1.73%) ⬇️
src/libsync/filesystem.cpp 73.11% <0.00%> (-1.08%) ⬇️
src/common/syncjournaldb.cpp 77.63% <0.00%> (-0.30%) ⬇️
src/libsync/account.h 33.33% <0.00%> (ø)
src/libsync/syncengine.h 50.00% <0.00%> (ø)
src/libsync/discoveryphase.h 25.00% <0.00%> (ø)
... and 4 more

@claucambra
Copy link
Collaborator Author

Note to self: will need incorporating changes from #5194

@claucambra claucambra force-pushed the feature/file-details-in-tray branch 2 times, most recently from 942695b to 38c7fad Compare November 30, 2022 18:00
@claucambra claucambra force-pushed the feature/file-details-in-tray branch 2 times, most recently from 625f5d3 to afb32b3 Compare December 9, 2022 12:42
@allexzander
Copy link
Collaborator

@claucambra I am not able to open the tray with this PR.

@claucambra
Copy link
Collaborator Author

@claucambra I am not able to open the tray with this PR.

rebase issue, fixed now

…ialog

Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
…-right

Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5139-5930fb4ac912d33ba431a017cef103c9cf8cbc6c-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarcloud
Copy link

sonarcloud bot commented Dec 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

11.1% 11.1% Coverage
0.0% 0.0% Duplication

@claucambra claucambra merged commit 659ecec into master Dec 9, 2022
@claucambra claucambra deleted the feature/file-details-in-tray branch December 9, 2022 16:44
@mgallien mgallien added this to the 3.7.0 milestone Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate file details into the tray window
6 participants