Heatmap mini fix #13637

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf · 2020-11-19T09:35:03Z

This fixes #13634 in a way - might not be a proper fix but the heatmap is now properly sized and also shown below 1200w.

* issue introduced by go-gitea#13623 makes the heatmap to show up really small

this might be a temporary fix

6543 · 2020-11-19T09:36:36Z

regression of #13623

codecov-io · 2020-11-19T10:08:22Z

Codecov Report

Merging #13637 (4383c2f) into master (ad2a288) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #13637      +/-   ##
==========================================
- Coverage   42.20%   42.19%   -0.01%     
==========================================
  Files         697      697              
  Lines       76577    76577              
==========================================
- Hits        32316    32311       -5     
- Misses      38960    38964       +4     
- Partials     5301     5302       +1

Impacted Files	Coverage Δ
modules/util/timer.go	`42.85% <0.00%> (-42.86%)`	⬇️
modules/charset/charset.go	`68.53% <0.00%> (-4.50%)`	⬇️
services/pull/pull.go	`41.27% <0.00%> (+0.49%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad2a288...4383c2f. Read the comment docs.

silverwind

This fix isn't right. We need flexbox to vertically center and we want to hide the heatmap on smaller screens. Will check out later unless you find a proper solution. I think the <svg> element needs its size increased to fill the parent container.

silverwind · 2020-11-19T10:52:45Z

This appears to work for me:

#user-heatmap > svg {
  width: 100%;
  height: 100%;
}

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf · 2020-11-19T11:05:05Z

This fix isn't right. We need flexbox to vertically center and we want to hide the heatmap on smaller screens. Will check out later unless you find a proper solution. I think the <svg> element needs its size increased to fill the parent container.

why do you want to hide the heatmap on smaller screens?
it scales very well.

also this puts it all together.

#user-heatmap {
  ...
  display: inline;
  ...
}

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf · 2020-11-19T11:06:36Z

display: inline

display-flex

small screen display: inline

small screen display: flex

edit: added small-screen screenshots

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf · 2020-11-19T11:08:46Z

notice how contribution stats and the heatmap legend are aligned when using display: inline

This appears to work for me:

#user-heatmap > svg {
  width: 100%;
  height: 100%;
}

also, is that with the original setting or with my changes already?

silverwind · 2020-11-19T11:13:36Z

why do you want to hide the heatmap on smaller screens?

Try scaling the window even smaller. In the past there was a issue where it overflowed the container but I guess with width: 100% we can prevent that and always show it.

notice how contribution stats and the heatmap legend are aligned when using display: inline

display: flex is needed to center the placeholder text when heatmap fails to load. Try altering web_src/js/features/heatmap.js so it shows that error text from the catch handler and you will see.

is that with the original setting

Yes, original settings.

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf · 2020-11-19T11:27:46Z

why do you want to hide the heatmap on smaller screens?

Try scaling the window even smaller. In the past there was a issue where it overflowed the container but I guess with width: 100% we can prevent that and always show it.

notice how contribution stats and the heatmap legend are aligned when using display: inline

display: flex is needed to center the placeholder text when heatmap fails to load. Try altering web_src/js/features/heatmap.js so it shows that error text from the catch handler and you will see.

is that with the original setting

Yes, original settings.

only changing svg {width,height}: 100% doesn seem to keep the heatmap vertically centered, but you can also still see the vertical difference between the stats and the legend below 1200w (had to delete the hiding query), whereas when using inline it's nicely aligned.

let me try scaling down once again with my setting.

here with the display: inline on the wrapping user-heatmap div

without display: inline

display: inline

I think it looks ok (both in the Dashboard and Public Activity tab) on large as well as small displays.

silverwind · 2020-11-19T11:54:57Z

web_src/less/features/heatmap.less

  text-align: center;
  position: relative;
  min-height: 125px;
-  display: flex;
  align-items: center;
  justify-content: center;


If you remove display: flex (which I don't agree with), you can also remove align-items and justify-content because those properties only apply to flexbox.

Please check if the placeholder seen in #13623 (comment) is still centered. I still think the flexbox removal is unwarranted, it's the best method to center things.

the error message could be flex-centered once the error occurs, maybe by defining another heatmap-error class or sth.
let me see if I can trigger an error to verify what it looks like now.

You can add a throw new Error("fail") at a place like here:

gitea/web_src/js/features/heatmap.js

Line 13 in ad2a288

Please check if the placeholder seen in #13623 (comment) is still centered. I still think the flexbox removal is unwarranted, it's the best method to center things.

I still wouldn't say a centered error message is more important than a nicely aligned heatmap, don't know...
hopefully, we can do both.

the error message could be flex-centered

why not flex-center everything including the svg? I see no drawback.

which is not perfect but not the worst either and considering it's an edge case and the fact that when shown it is shown nicely, I'd think it's manageable.

@silverwind I think I got a remedy for your concern.

what do you say on 474119d ?
is that alright?

Not sure yet, need to test later.

""-->'' in 650964b
posted a screenshot below, too.

@silverwind

* address concerns of @silverwind and in case of an error add 'display: flex' to the heatmap element, resulting in a nicely centered error message (since we kept the centering attributes in heatmap.less)

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf · 2020-11-19T12:52:00Z

centered heatmap error message as per @silverwind 's request.

silverwind · 2020-11-19T16:39:18Z

This display: inline also regresses the reserved space for the element so the page does not jump around after loading because min-height does not work on display: inline and I generally think block level elements like the heatmap should not be made inline which is made for things that should behave like text.

I'll raise another PR.

Apparently SVG inside flexbox renders slightly different across browsers where Firefox would stretch to fit the parent while Chrome and safari wouldn't. Stretch the SVG to the width of the parent for consistent rendering. Also did a few minor tweaks on the min-height of the box so it takes up less space on smaller responsive breakpoints. Fixes: go-gitea#13634 Fixes: go-gitea#13637

* linter says gimme single quotes

silverwind · 2020-11-19T16:53:08Z

Alternative fix in #13645.

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf · 2020-11-19T16:57:40Z

Alternative fix in #13645.

hm let me try it.

Apparently SVG inside flexbox renders slightly different across browsers where Firefox would stretch to fit the parent while Chrome and safari wouldn't. Stretch the SVG to the width of the parent for consistent rendering. Also did a few minor tweaks on the min-height of the box so it takes up less space on smaller responsive breakpoints. Fixes: go-gitea#13634 Fixes: go-gitea#13637

* Fix heatmap rendering in Chrome and Safari Apparently SVG inside flexbox renders slightly different across browsers where Firefox would stretch to fit the parent while Chrome and safari wouldn't. Stretch the SVG to the width of the parent for consistent rendering. Also did a few minor tweaks on the min-height of the box so it takes up less space on smaller responsive breakpoints. Fixes: #13634 Fixes: #13637 * position tweak Co-authored-by: zeripath <art27@cantab.net>

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf added 2 commits November 19, 2020 09:19

fix: show dashboard heatmap in full size

3cb6c58

* issue introduced by go-gitea#13623 makes the heatmap to show up really small

fix: heatmap not showing up below 1200w

4383c2f

this might be a temporary fix

6543 requested a review from silverwind November 19, 2020 09:35

6543 added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Nov 19, 2020

6543 added this to the 1.14.0 milestone Nov 19, 2020

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 19, 2020

6543 approved these changes Nov 19, 2020

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 19, 2020

silverwind requested changes Nov 19, 2020

View reviewed changes

chore: nice heatmap alignment with display: inline

046d8c8

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf requested a review from silverwind November 19, 2020 11:41

silverwind reviewed Nov 19, 2020

View reviewed changes

chore: keep error message centering

474119d

* address concerns of @silverwind and in case of an error add 'display: flex' to the heatmap element, resulting in a nicely centered error message (since we kept the centering attributes in heatmap.less)

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf requested a review from silverwind November 19, 2020 12:53

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf force-pushed the heatmap-mini branch from 474119d to 650964b Compare November 19, 2020 12:56

lint: fix error when using double quotes

f2472b0

* linter says gimme single quotes

silverwind mentioned this pull request Nov 19, 2020

Fix heatmap rendering in Chrome and Safari #13645

Merged

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf force-pushed the heatmap-mini branch from 650964b to f2472b0 Compare November 19, 2020 16:55

techknowlogick closed this in #13645 Nov 23, 2020

go-gitea locked and limited conversation to collaborators Jan 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heatmap mini fix #13637

Heatmap mini fix #13637

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020

6543 commented Nov 19, 2020

codecov-io commented Nov 19, 2020

silverwind left a comment •

edited

Loading

silverwind commented Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020 •

edited

Loading

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020

silverwind commented Nov 19, 2020 •

edited

Loading

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020 •

edited

Loading

silverwind Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf Nov 19, 2020

silverwind Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf Nov 19, 2020

silverwind Nov 19, 2020 •

edited

Loading

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf Nov 19, 2020

silverwind Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020

silverwind commented Nov 19, 2020 •

edited

Loading

silverwind commented Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020

Heatmap mini fix #13637

Heatmap mini fix #13637

Conversation

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020

6543 commented Nov 19, 2020

codecov-io commented Nov 19, 2020

Codecov Report

silverwind left a comment • edited Loading

Choose a reason for hiding this comment

silverwind commented Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020 • edited Loading

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020

silverwind commented Nov 19, 2020 • edited Loading

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020

silverwind commented Nov 19, 2020 • edited Loading

silverwind commented Nov 19, 2020

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020

silverwind left a comment •

edited

Loading

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020 •

edited

Loading

silverwind commented Nov 19, 2020 •

edited

Loading

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf commented Nov 19, 2020 •

edited

Loading

silverwind Nov 19, 2020 •

edited

Loading

silverwind commented Nov 19, 2020 •

edited

Loading