Page MenuHomePhabricator

Limit the pop up size
Closed, ResolvedPublic3 Story Points

Description

Motivation
Reference previews can often, but not always show the whole contents of a footnote.

Design Specifications


Accceptance Criteria

  • Reference Preview pop ups always have the width as defined in the design spefications
  • Reference Preview pop ups always have a min width and a max width as defined in the design spefications
  • Between min and max width the reference preview should adapt to the content size
  • If the content size is bigger than what fits, scroll bars should be added as defined by the browsers

Notes

  • The fade out effect is not part of this story

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 18 2019, 2:02 PM
Lea_WMDE triaged this task as Normal priority.Jan 18 2019, 2:02 PM
Lea_WMDE set the point value for this task to 3.
Lea_WMDE moved this task from In preparation to Ready for pickup on the Reference Previews board.
WMDE-Fisch moved this task from Sprint Backlog to Doing on the WMDE-QWERTY-Sprint-2019-02-06 board.

Change 488938 had a related patch set uploaded (by WMDE-Fisch; owner: WMDE-Fisch):
[mediawiki/extensions/Popups@master] Adjust the reference popup size and enable scollbars

https://gerrit.wikimedia.org/r/488938

Change 490091 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Popups@master] Reduce scroll code for reference popups to the bare minimum

https://gerrit.wikimedia.org/r/490091

Change 490091 abandoned by Thiemo Kreuz (WMDE):
Reduce scroll code for reference popups to the bare minimum

Reason:
Squashed into Ifcc355f.

https://gerrit.wikimedia.org/r/490091

Change 488938 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Adjust the reference popup size and enable scollbars

https://gerrit.wikimedia.org/r/488938

thiemowmde added a subscriber: thiemowmde.

Here are screenshots of how the design looks like after https://gerrit.wikimedia.org/r/488938:

Notes:

  • The width is fixed to 320px, instead of the suggested 340px. This is hard-coded for all popup types, not easy to change, and probably not worth the effort in our opinion.
  • The minimum height (as seen in the first screenshot) is as small as it can be. There is no extra whitespace except the one that is absolutely needed. Note this is different from all other popup types that always reserve space for a second line of text.
  • The orange 15px measurements from the draft are hard to measure on real popups. We decided to not fiddle with individual pixels but keep all these measurements identical to how they have been implemented for regular page previews before.
  • We run into conflicts with the suggested max-height:
    • It makes reference previews appear surprisingly cramped. Even medium-size footnotes start showing scrollbars.
    • Regular page preview popups already have a max-height defined (see the second screenshot above). Why do reference popups need to be so much smaller than that?
    • We decided to not implement the suggested max-height for now, but again keep what was implemented for regular page previews already.

@alexhollender do Thiemo's suggested changes work for you?

...

  • We run into conflicts with the suggested max-height:
    • It makes reference previews appear surprisingly cramped. Even medium-size footnotes start showing scrollbars.
    • Regular page preview popups already have a max-height defined (see the second screenshot above). Why do reference popups need to be so much smaller than that?
    • We decided to not implement the suggested max-height for now, but again keep what was implemented for regular page previews already.

@thiemowmde I'm curious about this one. I implemented the max-height in the demo I created (link) and didn't run into the issue you mentioned with the scrollbar appearing quite often. It appears once the text is over 4 lines, which seems reasonable to me. We have received feedback about keeping these compact, which I believe is a good goal, so I think restricting the height is worth investing in.

Change 491942 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Popups@master] Limit reference popups to max. 5 lines instead of 7

https://gerrit.wikimedia.org/r/491942

Change 491942 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Limit reference popups to max. 5 lines instead of 7

https://gerrit.wikimedia.org/r/491942

thiemowmde removed WMDE-Fisch as the assignee of this task.Feb 21 2019, 11:37 AM
thiemowmde added a subscriber: WMDE-Fisch.

Thanks for the additional information. We had a good chat with WMDE-Design (hail to our new room layout), and believe the solution presented in https://gerrit.wikimedia.org/r/491942 should make everybody happy.

  • We wondered why the size of reference previews should be different from page previews, and found one striking argument: A page is a bigger, more massive thing than a reference. It makes sense to allow page popups to be more massive, but give reference popups a lighter appearance. This gives the user a subtle visual hint.
  • We are not sure where the "4" is coming from. It conflicts with all other information given. We believe this is a mistake and decided to stick to what the demos at https://reference-previews.firebaseapp.com/ show, as well as all the measurements given in the design document.
  • The design document asks for 106px for the content. The effective height as of https://gerrit.wikimedia.org/r/491942 is now 100px. We are not sure what the extra 6px are for and decided to ignore them for now. They would not do anything but make the code (containing an arbitrary + 6px) confusing.
  • The document suggests two slightly different maximum heights: 215px and 212px. The effective maximum height as implemented with https://gerrit.wikimedia.org/r/491942 is now 203px. Note this is a computed number derived from the line count, line height and all margins combined, not hard-coded anywhere in the code. If you think the current 203px are to small, we would need to adjust the line count or some margin (please tell us which) instead.

Result:

Next steps:

  • Create a separate ticket to implement the fade-out effect.
  • Do we need to change the scrollbars? Currently they are what the browser gives us.
  • We might need to reposition the scrollbar and move it a bit to the right. Currently the text is able to touch the scrollbar, which can look odd.
alexhollender added a comment.EditedFeb 21 2019, 1:06 PM

@thiemowmde

The design document asks for 106px for the content. The effective height as of https://gerrit.wikimedia.org/r/491942 is now 100px. We are not sure what the extra 6px are for and decided to ignore them for now. They would not do anything but make the code (containing an arbitrary + 6px) confusing.

The height of the preview, as well as the height of the content container, were carefully considered to balance the display of content with the space the preview takes up on the screen. Please follow the designs here.


The document suggests two slightly different maximum heights: 215px and 212px. The effective maximum height as implemented with https://gerrit.wikimedia.org/r/491942 is now 203px. Note this is a computed number derived from the line count, line height and all margins combined, not hard-coded anywhere in the code. If you think the current 203px are to small, we would need to adjust the line count or some margin (please tell us which) instead.

My apologies for the conflicting information — the correct value is 215px (you can inspect a preview in the demo to see this). I have compared the screenshot you've provided with the demo, and it seems like the smaller height of yours (aside from the 6px missing from the content) may be due to not enough spacing between the content and the footer link.


Do we need to change the scrollbars? Currently they are what the browser gives us.
We might need to reposition the scrollbar and move it a bit to the right. Currently the text is able to touch the scrollbar, which can look odd.

  • What options do we have for the look of the scrollbar?
  • Is your current work available somewhere that I can look at it?

Change 491971 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Popups@master] Increase whitespace between reference text and read link

https://gerrit.wikimedia.org/r/491971

@alexhollender, there is discussion about whether the bottom margin changes should apply to all previews, not just references. If I've done it right, here's what the old margins look like currently:

And the new bigger bottom margin:

They look a little large to me but what do you think?

They are too large as is.

I'm not sure what happened in the screenshot F28259096. When I apply the change currently proposed in https://gerrit.wikimedia.org/r/491971 to all popup types, this is what happens.

Reference preview (before/after):

Page preview (before/after):

Disambiguation preview (before/after):

These look a bit different because:

  • Page previews don't have a footer link.
  • Disambiguation previews do have a min-height of 2 lines for the content, but the English message is short enough to fit in 1 line. Note this drastically changes with every localization that is a bit longer, as demonstrated below.

Apologies for the confusion here. In T214169#4971741 I was not trying to push for a change to the bottom margins for all popups, rather I was trying to discover what the discrepancy might've been between the screenshot @thiemowmde provided and the ones in the demo. What is important here is the space that is allocated for the content of the reference previews, which I think we've agreed does not necessarily need to be the same height as the content area for other types of previews.

It might be worth documenting which parts of the previews are easy to customize per-type, and which aspects are shared amongst all previews and need to remain consistent. In the case of the footer, for reference previews there is a link and a settings icon, so the spacing used in page previews feels a bit tight. However if we increase them both then page previews feels like it has an unnecessarily large footer.

Change 491971 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Increase whitespace between reference text and read link

https://gerrit.wikimedia.org/r/491971

Lea_WMDE closed this task as Resolved.Mar 1 2019, 10:55 AM
Lea_WMDE claimed this task.
Lea_WMDE moved this task from Demo to Done on the WMDE-QWERTY-Sprint-2019-02-20 board.