-
Notifications
You must be signed in to change notification settings - Fork 76
EditCounter: change year/month chart height formula in subpages to account for the legend #502
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
Conversation
See https://phabricator.wikimedia.org/T311704#10753784 for details and rationale
had forgotten a few things
It's probably the same, but can't hurt.
|
Did some thorougher testing now that I can, I believe this is good to go. https://phabricator.wikimedia.org/T311704#10758172 has a screenshot of what the 60px case looks like. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #502 +/- ##
============================================
- Coverage 71.30% 70.96% -0.35%
- Complexity 1275 1293 +18
============================================
Files 46 46
Lines 3792 3892 +100
============================================
+ Hits 2704 2762 +58
- Misses 1088 1130 +42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MusikAnimal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using an external htmllegend, which not only would require a serious chartjs upgrade but would also be very messy, as we'd be recreating manually all of the legend's functionalities
It's a little known feature, but when viewing a full result page (example), you can hover over the rows in the "Namespace totals" table and toggle namespaces, which then updates the charts accordingly. Thus, the external HTML legend solution should be feasible. I agree though it would still be messy and probably not worth your while. What you have done in this PR is a clear improvement and I'm content calling it a day.
Thanks again for contributing! 😃
|
(I in fact did know that toggling namespaces work. That's one of the reason I didn't want to get the legend out, because it meant reimplementing this to work outside the chart as well as inside the chart. Or else we'd be losing this feature on the subpage. Which is meh.) |
|
Wow, I can get dense sometimes. Took me like a week of repeatedly criscrossing editcounter.js to realise that we already have an external legend 🤦. Perhaps later we should switch to that, as this formula was based on calculations, but eg if a wiki has 50 namespaces due to some extension or other, it'll break down. Made T392604 to do this. |
Essentially it's changing, on the year/month count subpages, the height formula from
25 * ec.yearCounts.yearLabels|length) + 30to25 * ( ec.yearCounts.yearLabels|length + ( (ec.yearCounts.totals|keys|length / 3)|round(1, "ceil") ) ).The issue is this: in the main page, the legend is centralised outside the year/month charts, so it's fine. In the subpages, though, it isn't, and so it takes some room.
Fine, you could say, there's the +30. But the legend can wrap! From testing, we can expect the legend to have from three to 10 namespaces per line (which means 25px for 3 to 10 namespaces). Even on 10 namespaces per line, with 26 namespaces and 3 years, this example is still perfectly unreadable.
Knowing we've got 1 to 30 namespaces and 1 to 30 rows, I've done a lot of thinking and calculating on this, and I think it should work. The idea is to accommodate for the worst possible case, so that the bars always stay visible.
As 3 to 10 namespaces per line is very different, this means that in the extremest case, on desktop, the bars can get a bit tall, so that on mobile they stay visible. Now, numbers: the worst possible case is 200px. But again, that's someone who edited 30 namespaces in 1 year/month; not going to happen often. On the real example of my year counts I linked to above, we'd get about 60px tall bars. And again, this will all be on desktop, and in the subpage; so 60px won't be too glaring huge. For the specific px/bar tables, see T311704.
And, by the way, to make these variations less awkward, I'm also proposing to remove the bar fixed height of 18px, so that it can accomodate for the actual line (the bar and its padding) being given a bit more or less than 25px exactly (which will always happen with some datasets, except if we eviscerate the legend).
This is not a very clean solution, granted; but it's better than what we have, and we don't have much better. (I looked a great deal into alternatives, and essentially it's either simply hiding the legend or using an external htmllegend, which not only would require a serious chartjs upgrade but would also be very messy, as we'd be recreating manually all of the legend's functionalities.)
This time as it's JS I've been able to do some testing on the measurements (downloading the page, deactivating some stuff, manually changing the height, seeing what it looks like and if it confirms hypothesises). The rounding is because if we just say 8.5px per namespace, then we'll only give 1 namespace 8.5px, even though it'll be 25px tall. (If this does end up requiring the dev setup I'm having trouble getting (see my comment on the other pull request on mysql connection issues), then at any rate we can just leave that here and pick it up later.)
Bug: T311704