Page MenuHomePhabricator

Add a frame and caption text to <graph>
Open, NormalPublic

Description

Just like in Kartographer, we need a frame (left/right/...) and an optional caption added to the graph result.

Related Objects

StatusAssignedTask
OpenNone
ResolvedNone
ResolvedCKoerner_WMF
DeclinedNone
OpenNone
OpenNone
OpenNone
OpenArlolra
OpenNone
DeclinedNone
OpenNone
OpenNone
ResolvedNone
DeclinedNone
ResolvedNone
OpenNone
OpenNone
StalledNone
Resolvedcscott
OpenNone
DeclinedNone
OpenNone
OpenNone
OpenNone

Event Timeline

Yurik created this task.Oct 10 2016, 3:12 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 10 2016, 3:12 AM
MaxSem claimed this task.Oct 11 2016, 10:51 PM
MaxSem moved this task from Backlog to In progress on the Maps-Sprint board.

Houston, we have a problem: graphs can work without explicit width/height declaration, or even ignore such declarations. This makes restricting caption width next to impossible. Demo of the problem: https://jsfiddle.net/e50umhp3/

Yurik moved this task from Backlog to Prioritized on the Graphs board.Oct 13 2016, 9:57 PM

Change 316033 had a related patch set uploaded (by MaxSem):
WIP: frames

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

MaxSem removed MaxSem as the assignee of this task.Oct 28 2016, 9:04 PM

Unassigning from myself, needs frontend attention.

JGirault moved this task from In progress to Needs review on the Maps-Sprint board.

Change 316033 had a related patch set uploaded (by MaxSem):
Frames and captions for graphs

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

Yurik added a comment.EditedOct 29 2016, 2:53 AM

@JGirault @MaxSem - the frame looks good, but I was not expecting the background color change. Do we want that? The graphs are drawn on a transparent canvas (unless graph sets a color). The frame does not appear as a "frame", but rather as a "canvas" of a gray color, so all graphs will now look different by default. I am not sure if this is a bad or good thing in itself, just a bit unexpected. Had we only added a thin frame, most of the graphs would have looked almost identical. I would love to either make background transparent (draw a thin frame line instead), or have some UI feedback on this before we announce it.

@JGirault @MaxSem - the frame looks good, but I was not expecting the background color change. Do we want that? The graphs are drawn on a transparent canvas (unless graph sets a color). The frame does not appear as a "frame", but rather as a "canvas" of a gray color, so all graphs will now look different by default. I am not sure if this is a bad or good thing in itself, just a bit unexpected. Had we only added a thin frame, most of the graphs would have looked almost identical. I would love to either make background transparent (draw a thin frame line instead), or have some UI feedback on this before we announce it.

What about putting a white background to the canvas container div?

Yurik added a comment.Oct 31 2016, 9:31 PM

I think we should do transparent, because on some wikis, especially the non-article pages use different background colors.

Option 1: With thumb background, + white background fallbackOption 2: With transparent background for the whole frame

In both cases, this is how it renders on mobile:

Mobile style is to have no borders and transparent background

Change 316033 had a related patch set uploaded (by JGirault):
Frames and captions for graphs

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

I'd prefer option 1 because it looks more like normal pictures and we would not need to establish a new "type of embeds" which looks differently which could lead to complicated community debates.

I'd prefer option 1 because it looks more like normal pictures and we would not need to establish a new "type of embeds" which looks differently which could lead to complicated community debates.

@T.seppelt, thanks for your opinion

To add a little technical note to this, I am reusing the same .thumb and .thumbinner CSS classes from picture thumbs, for both option 1 and option 2.
So even though these implementations look very different on screen, in terms of code, the visual difference between the two only consists in a couple of lines of CSS overrides.

I hope we will receive more feedback

Yurik added a comment.EditedNov 2 2016, 1:15 AM

@JGirault, what will happen if the graph is added in the middle of this page?
https://fr.wikipedia.org/wiki/Utilisateur:Yurik/Brouillon

P.S. assuming the graph itself does not set its background - thus it is transparent

@JGirault, what will happen if the graph is added in the middle of this page?
https://fr.wikipedia.org/wiki/Utilisateur:Yurik/Brouillon
P.S. assuming the graph itself does not set its background - thus it is transparent

Option 1: FrameOption 2: Frame
  • For the frameless mode, they currently look the same, but we can implement same white background fallback for option 1. I attached screenshots.
Option 1: FramelessOption 2: Frameless
Note that we could also fallback to a white background here, in which case will render as below:
Yurik added a comment.Nov 2 2016, 1:50 AM

@JGirault thanks for the great pics! I keep thinking that we should have the combination of the first and second (framed) - We should have two lines with darker gray fill in between like in the option1, but with the transparent background of the option 2. I don't think we should break user's expectation that transparent graph is really transparent, not white.

@JGirault thanks for the great pics! I keep thinking that we should have the combination of the first and second (framed) - We should have two lines with darker gray fill in between like in the option1, but with the transparent background of the option 2. I don't think we should break user's expectation that transparent graph is really transparent, not white.

Well, it's the end of the day here, so I may not see it correctly., but it sounds impossible at the moment.
Let me explain: if the graph is transparent, the background color becomes the parent's background color, i.e. grey (the frame), instead of blue (the frame's parent).
It requires the graph's parent element (the frame) to also be transparent, to retrieve the frame's parent blue background.

Yurik added a comment.Nov 2 2016, 2:03 AM

@JGirault Are you saying there is no way for a transparent html element to have a complex non-transparent border with two lines and dark gray in between? I don't know much about styling frames. Perhaps three nested objects, one with dark gray 1px frame, next is lighter gray 5px frame, and next one with dark gray 1px frame again, while each has transparent content?

@JGirault Are you saying there is no way for a transparent html element to have a complex non-transparent border with two lines and dark gray in between? I don't know much about styling frames. Perhaps three nested objects, one with dark gray 1px frame, next is lighter gray 5px frame, and next one with dark gray 1px frame again, while each has transparent content?

The caption text needs to be in the grey area, which I guess it can with a grey background applied on .thumbcaption. It feels a little hacky at first sight, but it may work, I'll give it a try !

@Yurik okay if we add one nested div.thumbborder then it works!
Here's how it looks with this method :

Yurik added a comment.Nov 4 2016, 4:37 AM

Awesome!!!!!

It looks great. Thank you!

Yurik added a subscriber: CKoerner_WMF.EditedNov 4 2016, 6:55 AM

@JGirault, also, is the right alignment still a todo? http://data.wmflabs.org/wiki/Graphs - i put some samples there, getting ready for @CKoerner_WMF 's announcement.

As for the behavior:

  • I think we should be consistent with <mapframe text="..."> instead of caption=

Lastly, we need to decide - to break or not to break:
Non breaking:

  • default: align=left, frameless
  • if text=is set, use align=right by default

Breaking:

  • default: align=right
JGirault added a comment.EditedNov 4 2016, 10:39 PM

With latest revision :

  • Framed vs Frameless rule:
    • Graphs are framed by default.
    • Add frameless attribute to prevent the graph from being framed.
  • Alignment: align attribute
FramedFrameless
!align (default)rightright
align=leftleftleft
align=centercentercenter
align=rightrightright
Screenshot
  • Extra information with text attributes
    • "caption" = specifies extra information about a graph, displayed below the graph, Later this text could be shown in the footer of a full screen dialog, similar as how Maps render in a full screen dialog.
    • "alt" = provides alternative information for a graph snapshot, if a user for some reason cannot view it.
      • If "alt" is not specified but "caption" is, the alternative text will be created automatically from "caption".
    • "title" = specifies extra information about a graph, displayed on hover when the graph is still static.
      • If "title" is not specified but "caption" is, the title text will be created automatically from "caption".
      • Deprecated in favor of "caption".
Yurik added a comment.Nov 7 2016, 6:49 AM

While we are on this topic, @JGirault et al, what do you think about citations and source links in graphs? We are getting closer to cross-wiki data support, like these multilingual examples. It would be good for the graph to show a link to its data sources, so that community can instantly see where data for the graph comes from (e.g. tabular data on Commons or a WDQS). Should we allocate some visual space for this as part of our graph frame work?

debt added a subscriber: debt.Nov 30 2016, 7:18 PM

Hi @CKoerner_WMF - this came up during our sprint planning...should this be one release or two and what do we need to do for community involvement on this as of now?

Thanks!

I'd say separate. This release/task is about the frame and attributes for the graph. Let's keep things separate and smaller to digest for folks. T149462 is ready to go if that's agreeable.

Someone from Design should review this before the changes go live. Pinging @Nirzar.

<bikeshedding>Personally, I think the new default appearance is a bad idea. Putting the graph in a grey box by default is so 2000s. We might as well go back to Monobook while we're at it ;) What's wrong with the current default appearance?</bikeshedding>

Yurik added a comment.Dec 18 2016, 2:44 AM

@kaldari the way we restructured the graph is so that it uses transparent background. So it is really up to the div element to set any color it wants. There are cases, like frwiki talk pages, which have complex striped background to indicate each level of conversation. I think the striped background would be bad, but it would be good to use whichever poster's comment's background happened to be.

@Yurik: I was basing my comment on the description at T149462. It says that the default appearance will change and gives the following screenshot:

Is that no longer accurate?

Yurik added a comment.Dec 19 2016, 6:26 PM

@kaldari I think the most relevant images are in T147768#2770637 and T147768#2773162.
I just deployed the latest version of this patch to http://data.wmflabs.org/wiki/Dimpvis_-_Fertility_vs_Life_Expectancy -- and it seems we still have some issues to resolve:

  • graph draws over caption
  • graph is smaller than needed (possibly a Vega related issue)

@Yurik: So the chart now has a frame around it no matter what? That seems even worse. The frame should be an option just like it is for images, not the default. Most of the time I would not want a frame around a chart, but it would be good to have as an option for cases where you just want to have a thumbnail version.

Yurik added a comment.Dec 19 2016, 8:51 PM

@kaldari, there is an option to disable the frame (frameless attribute i think). Do you think we should do it the same way as images - nothing by default, and with a frame on request? e.g. if frame attribute is set? In any case, this hasn't been merged yet.

@kaldari, we reused the styles for image frames, if you think they're obsolete you should change these.

@Yurik: Personally, I think having it (roughly) match the behavior of images would be preferable, but I'm glad to hear there is at least an option to make it frameless. I also feel like it still needs support for a title element (separate from a caption). Should I create a separate ticket for that?

@MaxSem: I was originally referring to the older screenshot, which was a different style. I'm fine with it just re-using the image styles.

@Yurik okay if we add one nested div.thumbborder then it works!
Here's how it looks with this method :

@Yurik @kaldari with the latest T154077, shouldn't we force a white background, for consistency?

Yurik added a comment.Jan 4 2017, 12:15 AM

Apparently there are some bugs and limitations in Vega that make predicting width/height ahead of time difficult. We might need to figure this out first, or figure out a way to create a frame that resizes on the fly. https://github.com/vega/vega/issues/691

debt changed the task status from Open to Stalled.Jan 4 2017, 7:39 PM

Change 316033 abandoned by MaxSem:
Frames and captions for graphs

Reason:
Due to technical problems that can be only addressed by upstream, no point in keeping this patch around if we can't merge it.

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

Deskana closed this task as Declined.Feb 6 2017, 7:45 PM
Deskana added a subscriber: Deskana.

Given that this should be addressed upstream, @JGirault and I believe this task should be declined.

Yurik reopened this task as Open.Mar 7 2017, 2:06 PM

Quite a few users have been requesting this. The Vega graphs already support this boxing mode, it just requires an extra param in the spec. @JGirault, what would happen if the actual image is bigger than the size you auto-detect? Will it autogrow? I think the best way to give this option to the users (literally two people asked me about it last night), is to make it optional instead of on by default. This way graph template authors can easily use this functionality when they design graphs in a way that will work with it.

debt removed JGirault as the assignee of this task.Jun 6 2017, 7:54 PM
debt moved this task from Stalled/Waiting to Backlog on the Maps-Sprint board.
debt removed a project: Patch-For-Review.

Moving to backlog until such time that we can take this up again.

Moving off the sprint board - the Discovery team won't be able to finish this work at this time.

The_RedBurn added a subscriber: The_RedBurn.

Like Yurik said in T147768#3080101, this has been fixed in Vega (2.6.5) since January 6.
What is preventing the update to Vega 2.6.5 (last 2.x version) (T174766)? Wouldn't it be both easier and preferable than a direct migration to Vega 3 (T165118)?

Lens0021 removed a subscriber: Lens0021.
Lens0021 added a subscriber: Lens0021.