Page MenuHomePhabricator
Create Task
Maniphest T214169

Limit the pop up size
Closed, ResolvedPublic3 Estimated Story Points
Actions

Assigned To
• Lea_WMDE
Authored By
• Lea_WMDE
Jan 18 2019, 2:02 PM
Tags
Referenced Files
F28280764: Screenshot from 2019-02-25 11-44-40_.png
Feb 25 2019, 11:04 AM
F28280932: Screenshot from 2019-02-25 11-57-17_.png
Feb 25 2019, 11:04 AM
F28280854: Screenshot from 2019-02-25 11-52-54_.png
Feb 25 2019, 11:04 AM
F28280832: Screenshot from 2019-02-25 11-50-35_.png
Feb 25 2019, 11:04 AM
F28259096: localhost_6006__selectedKind=Thumbnails&selectedStory=portrait&full=0&addons=1&stories=1&panelRight=1&addonPanel=storybooks/storybook-addon-knobs.png
Feb 21 2019, 8:39 PM
F28259101: localhost_6006__selectedKind=Thumbnails&selectedStory=portrait&full=0&addons=1&stories=1&panelRight=1&addonPanel=storybooks/storybook-addon-knobs (1).png
Feb 21 2019, 8:39 PM
F28257485: comparison.jpg
Feb 21 2019, 1:06 PM
F28257278: Screenshot from 2019-02-21 11-45-53.png
Feb 21 2019, 11:37 AM
View All 12 Files
Subscribers
Aklapper
• alexhollender_WMF
gerritbot
• Lea_WMDE
• Niedzielski
thiemowmde
Volker_E
WMDE-Fisch

Description

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

Design Specifications

design-specifications.pdf143 KBDownload

Bildschirmfoto 2019-01-18 um 14.56.00.png (1×2 px, 539 KB)

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

Details

SubjectRepoBranchLines /-
Increase whitespace between reference text and read linkmediawiki/extensions/Popupsmaster 1 -0
Limit reference popups to max. 5 lines instead of 7mediawiki/extensions/Popupsmaster 4 -1
Adjust the reference popup size and enable scollbarsmediawiki/extensions/Popupsmaster 12 -1
Reduce scroll code for reference popups to the bare minimummediawiki/extensions/Popupsmaster 4 -10
Customize query in gerrit

Related Objects

Event Timeline

• Lea_WMDE created this task.Jan 18 2019, 2:02 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 18 2019, 2:02 PM
• Lea_WMDE triaged this task as Medium 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 added a project: WMDE-QWERTY-Sprint-2019-02-06.Feb 6 2019, 1:44 PM
WMDE-Fisch moved this task from Ready for pickup to Doing on the Reference Previews board.
WMDE-Fisch claimed this task.Feb 7 2019, 12:30 PM
WMDE-Fisch moved this task from Sprint Backlog to Doing on the WMDE-QWERTY-Sprint-2019-02-06 board.
gerritbot subscribed.Feb 7 2019, 3:05 PM

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

gerritbot added a project: Patch-For-Review.Feb 7 2019, 3:05 PM
WMDE-Fisch moved this task from Doing to Review on the WMDE-QWERTY-Sprint-2019-02-06 board.Feb 7 2019, 3:05 PM
gerritbot added a comment.Feb 12 2019, 4:44 PM

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

gerritbot added a comment.Feb 12 2019, 5:15 PM

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

gerritbot added a comment.Feb 12 2019, 5:35 PM

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 moved this task from Review to Demo on the WMDE-QWERTY-Sprint-2019-02-06 board.Feb 12 2019, 5:42 PM
thiemowmde subscribed.

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

Screenshot from 2019-02-12 18-18-11.png (150×346 px, 18 KB)

Screenshot from 2019-02-12 18-18-17.png (265×336 px, 26 KB)

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.
ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.17; 2019-02-12).Feb 12 2019, 6:00 PM
• Lea_WMDE added a subscriber: • alexhollender_WMF.Feb 20 2019, 9:05 AM

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

• alexhollender_WMF added a comment.Feb 20 2019, 7:18 PM
In T214169#4948486, @thiemowmde wrote:

...

  • 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.

gerritbot added a comment.Feb 21 2019, 11:06 AM

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

gerritbot added a comment.Feb 21 2019, 11:19 AM

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 project: WMDE-QWERTY-Sprint-2019-02-20.
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:

Screenshot from 2019-02-21 11-45-53.png (226×341 px, 23 KB)

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.
thiemowmde moved this task from Sprint Backlog to Demo on the WMDE-QWERTY-Sprint-2019-02-20 board.Feb 21 2019, 11:37 AM
ReleaseTaggerBot edited projects, added MW-1.33-notes (1.33.0-wmf.19; 2019-02-26); removed MW-1.33-notes (1.33.0-wmf.17; 2019-02-12).Feb 21 2019, 12:01 PM
• alexhollender_WMF 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.

comparison.jpg (341×803 px, 77 KB)

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?
gerritbot added a comment.Feb 21 2019, 2:40 PM

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

• Niedzielski subscribed.Feb 21 2019, 8:39 PM

@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:

localhost_6006__selectedKind=Thumbnails&selectedStory=portrait&full=0&addons=1&stories=1&panelRight=1&addonPanel=storybooks/storybook-addon-knobs (1).png (1×3 px, 2 MB)

And the new bigger bottom margin:

localhost_6006__selectedKind=Thumbnails&selectedStory=portrait&full=0&addons=1&stories=1&panelRight=1&addonPanel=storybooks/storybook-addon-knobs.png (1×3 px, 2 MB)

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

Volker_E subscribed.Feb 21 2019, 8:42 PM

They are too large as is.

thiemowmde added a comment.Feb 25 2019, 11:04 AM

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):

Screenshot from 2019-02-25 11-52-54_.png (236×672 px, 26 KB)

Page preview (before/after):

Screenshot from 2019-02-25 11-44-40_.png (439×678 px, 170 KB)

Disambiguation preview (before/after):

Screenshot from 2019-02-25 11-50-35_.png (184×676 px, 11 KB)

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.

Screenshot from 2019-02-25 11-57-17_.png (175×671 px, 14 KB)

• alexhollender_WMF added a comment.Feb 26 2019, 1:08 AM

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.

gerritbot added a comment.Feb 26 2019, 4:17 PM

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.
ReleaseTaggerBot edited projects, added MW-1.33-notes (1.33.0-wmf.20; 2019-03-05); removed MW-1.33-notes (1.33.0-wmf.19; 2019-02-26).Mar 5 2019, 11:02 AM
Pols12 mentioned this in T241750: Consider extending popup size.Jan 2 2020, 5:49 PM
Maintenance_bot removed a project: Patch-For-Review.Jan 2 2020, 6:10 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits