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.

Sign up for GitHub

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

Jump to bottom

narrow_mode: Add option to hide user list on wide screens. #19927

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

narrow_mode: Add option to hide user list on wide screens. #19927

wants to merge 3 commits into from

Conversation

amanagr
Copy link
Member

@amanagr amanagr commented Oct 7, 2021

We store the css required to hide user list on narrow screens in
narrow_width_styles and apply them on wise screens if
narrow_mode is enabled in user_settings.

Declaring narrow_width_styles requires addition of
postcss-mixins packages. This helps keep codebase clean
of repeated code.

To apply the JS required to hide user list in wide screens we
make the app behave as if we were in narrow width on wide
widths by making is_narrow True in message_viewport.

narrow_mode field is added to UserProfile which is used
to toggle this option by the user.

@amanagr amanagr force-pushed the hide_right_sidebar branch 3 times, most recently from e2c6ec1 to 6690bd2 Compare October 7, 2021 13:44
@amanagr amanagr force-pushed the hide_right_sidebar branch 2 times, most recently from 6f9f0d6 to d8ac006 Compare October 8, 2021 18:20
@amanagr
Copy link
Member Author

amanagr commented Oct 8, 2021

Oh! Migrations conflict wait a sec.

@amanagr amanagr force-pushed the hide_right_sidebar branch 2 times, most recently from 20103b2 to 861857d Compare October 9, 2021 11:23
@amanagr
Copy link
Member Author

amanagr commented Oct 9, 2021

There is some issue with scrollbar width, which I am working on. Need to run this both on mac and window to properly test it.

@amanagr amanagr force-pushed the hide_right_sidebar branch 2 times, most recently from 1a9720e to 09f257d Compare November 1, 2021 11:31
@zulipbot zulipbot added size: M and removed size: XL labels Nov 1, 2021
@amanagr amanagr force-pushed the hide_right_sidebar branch 2 times, most recently from b4d8eb5 to 1a9720e Compare November 1, 2021 11:35
@zulipbot zulipbot added size: XL and removed size: M labels Nov 1, 2021
@amanagr
Copy link
Member Author

amanagr commented Nov 1, 2021 •
edited

This should be ready for review. Since the changes only apply if narrow_mode is enabled, should be a safe deploy. It is important this gets tested on different OS and browsers. Although, I tested it on Mac and windows myself.

@amanagr
Copy link
Member Author

amanagr commented Apr 21, 2022

image

There is one issue that I am aware of, that there is extra space on top of the expanded right-column. I can easily fix it, but I don't understand what change is causing it, so I am trying to figure that out.

@terpimost terpimost mentioned this pull request Apr 24, 2022
Closed
@amanagr
Copy link
Member Author

amanagr commented Apr 26, 2022

@alya this should be ready for your review. Thanks!

@alya
Copy link
Contributor

alya commented Apr 27, 2022 •
edited

Cool, thanks! A few observations:

  1. Turning on this option makes "Use full width on wide screens" stop working.
  2. Turning on this option breaks the upper right area when "Show user list on left sidebar in narrow windows" is selected

Screen Shot 2022-04-27 at 1 18 43 PM

3. I think we should have the following order for these three settings in the menu:

Use full width on wide screens
Hide user list on wide screens
Show user list on left sidebar in narrow windows

@amanagr amanagr force-pushed the hide_right_sidebar branch 5 times, most recently from d7c0757 to 22b96bc Compare May 2, 2022 02:22
@amanagr
Copy link
Member Author

amanagr commented May 2, 2022

@alya this is ready for another review. I fixed a lot of cases related to the three modes that I missed earlier.

@alya
Copy link
Contributor

alya commented May 25, 2022

Thanks!

I guess playing around with this combination of settings:
Screen Shot 2022-05-25 at 1 21 32 AM

It looks like we end up always showing the list in the left sidebar (including in wide windows). That doesn't quite seem to match what the settings say, but I also don't want us to spend much time on tuning this, since I expect the left sidebar option to be deprecated.

@alya
Copy link
Contributor

alya commented May 25, 2022

Thinking about the new option, should it just be called "Hide user list", or perhaps "Hide user list (right sidebar)" for clarity? Since the user list is hidden with all window sizes when this option is selected, it seems confusing to specifically mention wide windows.

@amanagr
Copy link
Member Author

amanagr commented May 26, 2022

Makes sense for the name change
Screenshot 2022-05-26 at 11 13 06 AM

@alya
Copy link
Contributor

alya commented Jun 9, 2022

@timabbott , we've done some iteration on this one -- I think the next step is a review from you.

@zulipbot
Copy link
Member

Heads up @amanagr, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

andersk added a commit to andersk/zulip that referenced this pull request Mar 20, 2023
@andersk
dependencies: Remove unused postcss-mixins plugin.
e7a065b 
Commit 7fc191d added this as
preparation for zulip#19927, but we already have @extend from
postcss-extend-rule and don’t need both.

Signed-off-by: Anders Kaseorg <anders@zulip.com>
timabbott pushed a commit that referenced this pull request Mar 20, 2023
@andersk @timabbott
dependencies: Remove unused postcss-mixins plugin.
577d410 
Commit 7fc191d added this as
preparation for #19927, but we already have @extend from
postcss-extend-rule and don’t need both.

Signed-off-by: Anders Kaseorg <anders@zulip.com>
andersk
andersk reviewed Mar 20, 2023
View reviewed changes
postcss.config.js
@@ -15,6 +15,7 @@ module.exports = ({file}) => ({
}),
require("postcss-nested"),
require("postcss-extend-rule"),
require("postcss-mixins"),
Copy link
Member

Choose a reason for hiding this comment

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

FYI in case this gets revived, I’ve removed postcss-mixins in #24731, because you can already do this with postcss-extend-rule (as we do for %dark-theme).

@miguelraz
Copy link

Sorry to poke @timabbott but it would be fantastic to get this through the finish line.

Appreciate all the hard work! <3

@amanagr
Copy link
Member Author

amanagr commented Jul 13, 2023

Latest discussion - https://chat.zulip.org/#narrow/stream/101-design/topic/hide.20userlist.20on.20wide.20screens.20.2321638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andersk andersk andersk left review comments

Assignees
No one assigned
Labels
has conflicts size: XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@amanagr @alya @timabbott @zulipbot @miguelraz @andersk