Overlay side-panel in autohide mode. #1100
Conversation
Rohitth007
force-pushed
the
panel-overlay
branch
from
47177c1 to
e90791a
Compare
Rohitth007
added
PR needs review
Rohitth007
force-pushed
the
panel-overlay
branch
6 times, most recently
from
97019dd to
9bd5323
Compare
There was a problem hiding this comment.
@Rohitth007 Thanks for working on this - it seems that people have generally come out in support of the change 👍
The feature works fine; my feedback is mainly focused on clarifying the implementation.
| # FIXME: This can be avoided after fixing the "sacrificing 1st | ||
| # unread msg" issue and setting focus_column=1 when initializing. | ||
| self.body.focus_position = 1 |
There was a problem hiding this comment.
Could you explain this further?
There was a problem hiding this comment.
@neiljp The best way to understand this is to comment out the line and run it.
Basically keypress pretty much handles navigation focus change for us but except when initialization we make the left_tab as the focus_column (which is actually not a selectable widget but can be forcefully focused like we did)
To work around that we have to initialize focus_column as center_panel but that would mean a message would be read at startup. Hence, the FIXME.
The reordering below is a result of this change. Though it actually works fine without it, the tests broke hence the reordering.
The reordering actually also connects the 2nd commit in #1072 (redirecting keypress)
There was a problem hiding this comment.
I mentioned this in the stream, but while I can confirm this does resolve one problem with setting focus, it doesn't solve opening compose or message-search from a panel, and I suggest we may want to use get/set_focus_path for this in general.
| self.body.focus_position = 2 | ||
| self.users_view.keypress(size, key) | ||
| self.show_left_panel(visible=False) | ||
| self.show_right_panel(visible=True) | ||
| self.body.focus_position = 2 | ||
| self.users_view.keypress(size, key) |
There was a problem hiding this comment.
I assume this move is related to the focus setting in the panel methods - which we've just removed?
(this now overrides that being set there)
There was a problem hiding this comment.
Yes, it's because of the override but this was not the one we removed in one of the previous refactor commits. That was focus_setting when visible = True this is when visible = False
neiljp
added
PR awaiting update
Rohitth007
force-pushed
the
panel-overlay
branch
from
9bd5323 to
2637561
Compare
Rohitth007
added
PR needs review
There was a problem hiding this comment.
@Rohitth007 The first two commits are fine refactors, there are just a few challenges left with the overlay UI as per the stream.
| # FIXME: This can be avoided after fixing the "sacrificing 1st | ||
| # unread msg" issue and setting focus_column=1 when initializing. | ||
| self.body.focus_position = 1 |
There was a problem hiding this comment.
I mentioned this in the stream, but while I can confirm this does resolve one problem with setting focus, it doesn't solve opening compose or message-search from a panel, and I suggest we may want to use get/set_focus_path for this in general.
neiljp
added
PR awaiting update
Rohitth007
force-pushed
the
panel-overlay
branch
4 times, most recently
from
98baa8f to
06a6055
Compare
This commit refactors `main_window` to use `show_left_panel` to show the left panel at startup instead of hardcoding it in `body`. Tests updated.
This commit stores the Fame widget of View as an attribute so that it can be easily manipulated for various purposes like overlaying widgets , adding padding, etc. Tests added.
This commit makes sure the side-panels close in all cases where the focus is set to center_panel, i.e., * special narrow hotkeys * message search * message compose (pm & stream) * open draft Tests updated.
This commit combines the tests for show_panel methods as they share a lot of commonalities.
This commit makes the side panels overlay the body which avoids squashing and stretching of the message view, which improves UX in autohide mode. To do this the `show_panel` functions have been updated to use Overlay. The focus setting in the `show_panel` functions which could be avoided at a later point needed a reordering in the SEARCH keypress which actually makes it the correct order, i.e., * first handle autohide * then handle search Tests updated.
|
@Rohitth007 Thanks for the work that's gone into this! I've only slightly adjusted the overlay options list to make the code more compact, and am now merging 🎉 |
neiljp
added
area: UI
What does this PR do?
This commit makes the side panels to overlay the body which avoids
squashing and stretching of the message view which improves UX in
autohide mode.
Tested?
Commit flow
Notes & Questions
Built on PR #1097
Interactions
Waiting on PR #1097
Blocks PR #1074
Visual changes
