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

Make UserStatusSelector a dismissible page pushed onto the tray window #4760

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

claucambra
Copy link
Collaborator

@claucambra claucambra commented Jul 20, 2022

Closes #3819

Screen.Recording.2022-07-22.at.13.39.14.mov

@claucambra claucambra self-assigned this Jul 20, 2022
@claucambra claucambra force-pushed the feature/modal-userstatusselector branch 3 times, most recently from 219e5d7 to 76e8287 Compare July 20, 2022 18:57
@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #4760 (d86f25d) into master (b90e79a) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4760      +/-   ##
==========================================
- Coverage   57.24%   57.19%   -0.05%     
==========================================
  Files         138      138              
  Lines       17144    17144              
==========================================
- Hits         9814     9806       -8     
- Misses       7330     7338       +8     
Impacted Files Coverage Δ
src/libsync/filesystem.cpp 73.11% <0.00%> (-3.23%) ⬇️
src/libsync/propagatedownload.cpp 64.00% <0.00%> (-1.19%) ⬇️
src/libsync/discovery.cpp 84.86% <0.00%> (+0.29%) ⬆️

@claucambra claucambra force-pushed the feature/modal-userstatusselector branch 4 times, most recently from 4da94f8 to 74cae56 Compare July 20, 2022 21:48
@mgallien
Copy link
Collaborator

the behavior with respect to having it close when we ant is much better
but currently this is not really working well as we now get a popup in which you have to scroll
so I would suggest to either make it again a real window that can be bigger than the main dialog but really behaves like a popup (close on lost focus) or have the content be much smaller such that we do not need to scroll

@claucambra
Copy link
Collaborator Author

the behavior with respect to having it close when we ant is much better
but currently this is not really working well as we now get a popup in which you have to scroll
so I would suggest to either make it again a real window that can be bigger than the main dialog but really behaves like a popup (close on lost focus) or have the content be much smaller such that we do not need to scroll

How about making the pre-defined statuses list scrollable?

@claucambra claucambra force-pushed the feature/modal-userstatusselector branch from 74cae56 to 41dfa8e Compare July 21, 2022 17:51
@claucambra
Copy link
Collaborator Author

claucambra commented Jul 21, 2022

the behavior with respect to having it close when we ant is much better
but currently this is not really working well as we now get a popup in which you have to scroll
so I would suggest to either make it again a real window that can be bigger than the main dialog but really behaves like a popup (close on lost focus) or have the content be much smaller such that we do not need to scroll

Screen.Recording.2022-07-21.at.19.50.54.mov

@mgallien
Copy link
Collaborator

@claucambra thanks for the video
to be honest I am not the best with regard to design of a solution but it is already much better that way
I also agree that the interaction with the set status window is much better than when it was just a plain window that could stay open if you did not find the proper action to close it

@nimishavijay
Copy link
Member

This is much better than the separate window :) I am not sure about the scrollable status list, it doesn't seem obvious that there are more statuses available and it seems a but cramped.

What do you think about replacing the main dialogue with the status dialogue instead of overlaying it? Would that be possible? It can then be in full height without any scrolling, and on clicking "Set status" or "Cancel" it could go back to the main dialogue. The position would depend on the OS, for eg. on Windows it would be bottom and right aligned with the main dialogue (because the tray menu icon is on the bottom right) and on Ubuntu it could be top and right aligned (assuming the tray menu icon is on the top right).

What do you think? cc @jancborchardt

@claucambra
Copy link
Collaborator Author

This is much better than the separate window :) I am not sure about the scrollable status list, it doesn't seem obvious that there are more statuses available and it seems a but cramped.

What do you think about replacing the main dialogue with the status dialogue instead of overlaying it? Would that be possible? It can then be in full height without any scrolling, and on clicking "Set status" or "Cancel" it could go back to the main dialogue. The position would depend on the OS, for eg. on Windows it would be bottom and right aligned with the main dialogue (because the tray menu icon is on the bottom right) and on Ubuntu it could be top and right aligned (assuming the tray menu icon is on the top right).

What do you think? cc @jancborchardt

I think an easier and maybe more intuitive way of doing this is by making the status selector a page that gets pushed on top of the existing dialog, forming a stack. Then this page can be removed by hitting a back or cancel button. This should have the same effect as your proposal without having ti create an entirely separate window -- thoughts? :)

@nimishavijay
Copy link
Member

I think an easier and maybe more intuitive way of doing this is by making the status selector a page that gets pushed on top of the existing dialog, forming a stack. Then this page can be removed by hitting a back or cancel button. This should have the same effect as your proposal without having ti create an entirely separate window -- thoughts? :)

Yes, that's exactly what I had in mind when I proposed that :)
We just have to make sure that it doesn't look too cramped, ideally there wouldn't be any scrolling required

@claucambra claucambra force-pushed the feature/modal-userstatusselector branch from 41dfa8e to 4adfc4c Compare July 22, 2022 11:45
@claucambra
Copy link
Collaborator Author

I think an easier and maybe more intuitive way of doing this is by making the status selector a page that gets pushed on top of the existing dialog, forming a stack. Then this page can be removed by hitting a back or cancel button. This should have the same effect as your proposal without having ti create an entirely separate window -- thoughts? :)

Yes, that's exactly what I had in mind when I proposed that :) We just have to make sure that it doesn't look too cramped, ideally there wouldn't be any scrolling required

How about this?

Screen.Recording.2022-07-22.at.13.39.14.mov

@claucambra
Copy link
Collaborator Author

@mgallien @allexzander @camilasan the diffs have blown up but this is mainly just due to moving what used to be most of the contents of Window.qml into a separate MainTrayView.qml, now that we have a StackView

@claucambra claucambra changed the title Make UserStatusSelector a modal popup Make UserStatusSelector a dismissible page pushed onto the tray window Jul 22, 2022
@claucambra claucambra force-pushed the feature/modal-userstatusselector branch 2 times, most recently from e19133f to eccd780 Compare July 22, 2022 11:56
Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Super nice! Looks great 🚀🚀🚀 :)

@jancborchardt
Copy link
Member

The design is much better! :)

The animation is a bit out of place though, as normally left/right animations denote diving down into details and going back up.

In this case, an animation from the bottom like an iOS/Android bottom sheet commonly used for dialogs would probably be more fitting. Does that work?

@claucambra claucambra force-pushed the feature/modal-userstatusselector branch from eccd780 to 7df7e93 Compare July 22, 2022 12:51
@claucambra
Copy link
Collaborator Author

The design is much better! :)

The animation is a bit out of place though, as normally left/right animations denote diving down into details and going back up.

In this case, an animation from the bottom like an iOS/Android bottom sheet commonly used for dialogs would probably be more fitting. Does that work?

Sure, I added this animation:

Screen.Recording.2022-07-22.at.14.57.45.mov

@mgallien
Copy link
Collaborator

The design is much better! :)

The animation is a bit out of place though, as normally left/right animations denote diving down into details and going back up.

In this case, an animation from the bottom like an iOS/Android bottom sheet commonly used for dialogs would probably be more fitting. Does that work?

@claucambra given the comment about having the set user status slide from bottom, I would go with a Drawer that are made for that (and inherit from Popup that was your original idea)
that way, you do not need to use a StackLayout but simply benefit from the Drawer existing behavior

@claucambra claucambra force-pushed the feature/modal-userstatusselector branch from 7df7e93 to 3ec1fac Compare July 27, 2022 11:44
@claucambra
Copy link
Collaborator Author

The design is much better! :)
The animation is a bit out of place though, as normally left/right animations denote diving down into details and going back up.
In this case, an animation from the bottom like an iOS/Android bottom sheet commonly used for dialogs would probably be more fitting. Does that work?

@claucambra given the comment about having the set user status slide from bottom, I would go with a Drawer that are made for that (and inherit from Popup that was your original idea) that way, you do not need to use a StackLayout but simply benefit from the Drawer existing behavior

Agree, looking at it now it is pretty obvious the drawer does what we want more directly

I have also rolled back the bigger changes of putting the tray window contents into its own file as I felt it was a bit out of scope

@claucambra claucambra force-pushed the feature/modal-userstatusselector branch from 3ec1fac to 2eef125 Compare July 27, 2022 11:48
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.

If the video in your comment is the current state: Nice! Really great job, looks slick :)

@claucambra claucambra force-pushed the feature/modal-userstatusselector branch from 2eef125 to d03a6ee Compare August 4, 2022 12:50
@allexzander
Copy link
Contributor

allexzander commented Aug 5, 2022

@claucambra Looks good. But, should not this dialog auto-close when the tray window gets closed by clicking outside it? For me (on Windows), the dialog remains open when I click that tray icon next time. I'd expect it to get closed if I changed my mind about setting the status.

#Update

I have also noticed one other bug. Selecting the status Away does not work now if the current status is Online. When selecting the Away status, it gets reset back to Online, when opening the dialog again.

@mgallien
Copy link
Collaborator

mgallien commented Aug 5, 2022

I tested it on my dev laptop
I noticed two issues
the layout can still overflow and would want to use more space than available
image
if the main dialog closes due to lost focus, the set status drawer will stay on top. that is unexpected and I would expect it to be closed

@claucambra
Copy link
Collaborator Author

@claucambra Looks good. But, should not this dialog auto-close when the tray window gets closed by clicking outside it? For me (on Windows), the dialog remains open when I click that tray icon next time. I'd expect it to get closed if I changed my mind about setting the status.

#Update

I have also noticed one other bug. Selecting the status Away does not work now if the current status is Online. When selecting the Away status, it gets reset back to Online, when opening the dialog again.

I think this will be caused by the model not being correctly requested to load when the drawer is opened. Will look into it, thanks

I tested it on my dev laptop
I noticed two issues
the layout can still overflow and would want to use more space than available

Argh. Okay, will test with different font sizes

@claucambra claucambra force-pushed the feature/modal-userstatusselector branch from d03a6ee to 9fcda24 Compare August 5, 2022 15:53
@claucambra
Copy link
Collaborator Author

@claucambra Looks good. But, should not this dialog auto-close when the tray window gets closed by clicking outside it? For me (on Windows), the dialog remains open when I click that tray icon next time. I'd expect it to get closed if I changed my mind about setting the status.
#Update
I have also noticed one other bug. Selecting the status Away does not work now if the current status is Online. When selecting the Away status, it gets reset back to Online, when opening the dialog again.

I think this will be caused by the model not being correctly requested to load when the drawer is opened. Will look into it, thanks

@allexzander This is actually used by a very, ahem, interesting bug outside the changes of this PR. Will open a new PR with a fix for this soon

@claucambra
Copy link
Collaborator Author

@claucambra Looks good. But, should not this dialog auto-close when the tray window gets closed by clicking outside it? For me (on Windows), the dialog remains open when I click that tray icon next time. I'd expect it to get closed if I changed my mind about setting the status.
#Update
I have also noticed one other bug. Selecting the status Away does not work now if the current status is Online. When selecting the Away status, it gets reset back to Online, when opening the dialog again.

I think this will be caused by the model not being correctly requested to load when the drawer is opened. Will look into it, thanks

@allexzander This is actually used by a very, ahem, interesting bug outside the changes of this PR. Will open a new PR with a fix for this soon

Fix in #4822

@claucambra claucambra force-pushed the feature/modal-userstatusselector branch 4 times, most recently from b60342a to 02909e4 Compare August 5, 2022 19:56
@claucambra
Copy link
Collaborator Author

@allexzander @mgallien The drawer now auto-closes when the tray window is closed, and I have adjusted the layout to remove unnecessary padding around certain elements and to reduce the spacing if there is insufficient space. This should really help

@camilasan
Copy link
Member

/rebase

Signed-off-by: Claudio Cambra <claudio.cambra@gmail.com>
@nextcloud-command nextcloud-command force-pushed the feature/modal-userstatusselector branch from 02909e4 to d86f25d Compare August 10, 2022 09:22
@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-4760-d86f25d21536e6953d7eb8cf22a33ee3c3c1911f-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 Aug 10, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

16.7% 16.7% Coverage
0.0% 0.0% Duplication

@claucambra claucambra merged commit e92aa62 into master Aug 10, 2022
@claucambra claucambra deleted the feature/modal-userstatusselector branch August 10, 2022 09:54
@mgallien mgallien added this to the 3.6.0 milestone Sep 2, 2022
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.

Make user status selector dialog modal
7 participants