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

Enable responsive search results if the new design is used (fixes #2839) #2855

Merged
merged 7 commits into from
Oct 23, 2021

Conversation

Yorwba
Copy link
Contributor

@Yorwba Yorwba commented Oct 1, 2021

This PR allows authenticated users to benefit from the more mobile-friendly layout if they have enabled the new design.

Two issues using the sentence menu on narrow screen are fixed with a slight redesign.

Expanded menu

Before

Screen width 900 px
Expanded menu before, at screen width of 900 pixels
Screen width 400 px
Expanded menu before, at screen width of 400 pixels
Screen width 300 px
Expanded menu before, at screen width of 300 pixels

After rearranging the menu

Screen width 900 px
Expanded menu after, at screen width of 900 pixels
Screen width 400 px
Expanded menu after, at screen width of 400 pixels
Screen width 300 px
Expanded menu after, at screen width of 300 pixels

I also tried smaller screens (200 px), but then something else hits its minimum width, preventing the page from shrinking further.

Collapsed menu

At 300 px, the problem caused by placing the sentence description ("sentence x belongs to y") next to the menu (already mentioned by jiru on the original PR) becomes quite severe:

Before

Collapsed menu before, at screen width of 300 px

I think a good solution is to allow the menu to wrap below the description:

After

Collapsed menu after, at screen width of 300 px

@trang trang modified the milestones: 2021-10-17, 2021-10-23 Oct 11, 2021
<div layout="row" class="header">
<md-subheader flex ng-if="!vm.isMenuExpanded">
<div layout="row" layout-align="end" layout-wrap class="header">
<md-subheader flex="auto" ng-if="!vm.isMenuExpanded">
Copy link
Member

Choose a reason for hiding this comment

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

@Yorwba Thanks for working on this and help move forward the transition to a responsive UI!

Rather than showing the sentence menu below the ID and owner, I think it feels more natural to have it at the very top. I actually had a commit in my half-dead pull request that did just that, by switching the layout to columnwhen the menu is expanded and using flex-order to put the menu element above the ID+owner element.

Taking that commit and with additional changes to ensure the menu is always full-width in small screen and that the change doesn't affect non-authenticated users, we could have something like this:

  • in sentence_and_translation.ctp:
        <?php
        if (CurrentUser::isMember()) {
            $layout = 'layout="{{vm.isMenuExpanded ? \'column\' : \'row\'}}" layout-xs="column"';
        } else {
            $layout = 'layout="row"';
        }
        ?>
        <div <?= $layout ?> class="header">
            <md-subheader flex flex-order="0">
  • and in sentence_menu.ctp:
<div class="menu-wrapper" sentence-menu 
     flex="{{vm.isMenuExpanded ? '100' : 'none'}}" flex-order="{{vm.isMenuExpanded ? -1 : 1}}"
     flex-xs="100" flex-order-xs="-1"
     ng-init="vm.initMenu(<?= (int)$expanded ?>, vm.sentence.permissions)">

If that looks fine to you, I'll let you incorporate it to your pull request, or do any additional adjustment if needed. Then it should be good for merge 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review.

I agree that putting the menu on top is more natural. I also noticed that you removed the ng-if="!vm.isMenuExpanded" from the ID+owner element, which is probably for the best when we have another way to make room for it.

However, I don't quite like using the -xs attributes to change the layout for small screens. The nice thing about the wrapping-based solution is that it takes the available space into account and switches to two rows exactly when a single row no longer fits.

I think there's a way to have the first element wrap to the second row instead of the last, which would also put the menu on top, but only when necessary. So I'll try that first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, the way to keep the menu on top with wrapping is to reverse the ordering of the elements in the HTML and then apply flex-direction: row-reverse to visually reverse them again, so that it appears as if nothing changed ... as long as everything fits on a single row. If it breaks into two rows, the menu stays on top and the id+username ends up below.

See the new commits.

@trang
Copy link
Member

trang commented Oct 20, 2021

@Yorwba Thanks for the updated solution!

There's just a small visual issue with this, caused by the rounded corner in the menu. It creates a dent in the middle of the menu bar. It's not super noticeable with the light grey background, so in my screenshot below I changed the color to make it more apparent:

image

Possible solutions

  1. Remove the rounded corner. The rounded corner gives a more "comfy" feel to the menu, but it's really not high impact in terms of UX and probably very few people notice it.
    image

  2. Remove the background color on .sentence-and-translations .header. I personally find the white space a bit awkward but again, the menu background is a very, very light grey so, it's probably not a big deal either.
    image

Maybe you have other ideas, but in any case, I don't consider this to be a blocking issue for merging. I would just merge this on Saturday and probably remove the rounded corner if you don't have time to look into it.

@Yorwba
Copy link
Contributor Author

Yorwba commented Oct 22, 2021

The rounded corner gives a more "comfy" feel to the menu, but it's really not high impact in terms of UX and probably very few people notice it.

Count me among the people who never noticed it, but now that you pointed it out, I cannot unsee it. :) It does feel a bit softer somehow.

Remove the background color on .sentence-and-translations .header. I personally find the white space a bit awkward but again, the menu background is a very, very light grey so, it's probably not a big deal either.

I think it makes sense to have the menu background fit snugly around the icons. That way, when the menu is expanded, it expands naturally into the whitespace. It does look a bit awkward, but having the background cover the entire top of the header with only a handful of icons is also awkward. So I like this option better.

I've taken the liberty to implement option 2 and also moved the flex-direction from the style attribute to the standalone CSS file, which is where I should have put it in the first place.

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.

Pages that are responsive when logged out are not responsive when logged in
2 participants