Skip to content
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

Fix heatmap rendering in Chrome and Safari #13645

Merged
merged 3 commits into from
Nov 23, 2020

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Nov 19, 2020

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

image

image

image

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
@silverwind silverwind mentioned this pull request Nov 19, 2020
@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

still getting small heatmaps in chromium and no heatmaps below 1200w with this.
image
image
image

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

Must be cached. Try clearing browser cache or start with RUN_MODE=dev.

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

Must be cached. Try clearing browser cache or start with RUN_MODE=dev.

cleared cache multiple times, cleared site data in browser, reloaded gitea in dev mode.
still small and hidden below 1200w.
image

edit: let me rebuild gitea once more

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

alright now this works for me fine and also scales well just as presented.
image

what is the advantage of hardocoding more values, though, e.g. compared to #13637 ?

@silverwind
Copy link
Member Author

silverwind commented Nov 19, 2020

The static min-height values are necessary if we want to prevent the page from jumping after load, I see no way around it because CSS can't know beforehand how big the SVG will render.

@GiteaBot 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
@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

The static min-height values are necessary if we want to prevent the page from jumping after load, I see no way around it because CSS can't know beforehand how big the SVG will render.

alright..
I still think the display: inline does the same thing and with style.
the only thing missing is the here-added min-heights so that it doesn't jump either.

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

noticed sth else though.
see what the titlebar does now along with the feed (where also heatmap is placed).
image
looks like it's shrinking preliminarily where it could still fit content, doesn't it?

.total-contributions {
font-size: 10px;
left: 17px;
bottom: -2px;
bottom: -4px;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

liked this better aligned with inline

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is absolutely positioned, display: inline on the parent wouldn't do anything.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is absolutely positioned, display: inline on the parent wouldn't do anything.

not here but on the user-heatmap div and I think it's more elegant that way.
then this would not be needed.
but if it works, it works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works fine for me.

@silverwind
Copy link
Member Author

silverwind commented Nov 19, 2020

I still think the display: inline does the same thing and with style.

Not sure why you persist on inline, it's just not meant for non-text content and thing like min-height would not work on it because that works only on block-level (you need line-height for inline elements at which point you're just enlarging one "text line" with no text on it).

see what the titlebar does now along with the feed (where also heatmap is placed).

I can't reproduce that one and it's certainly not related here.

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

I still think the display: inline does the same thing and with style.

Not sure why you persist on inline, it's just not meant for non-text content and thing like min-height would not work on it because that works only on block-level (you need line-height for inline elements at which point you're just enlarging one "text line" with no text on it).

see my comment above.
basically boils down to - do what you think is best.

see what the titlebar does now along with the feed (where also heatmap is placed).

I can't reproduce that one and it's certainly not related here.

no, it's a separate issue but I think may also be affecting the feed column, of which heatmap is a part.
as a whole it might look even better once that is fixed.
just noticed it now, would need investigation.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 19, 2020
@mrsdizzie mrsdizzie added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Nov 21, 2020
@silverwind
Copy link
Member Author

This should be good to go.

@bkraul bkraul mentioned this pull request Nov 23, 2020
4 tasks
@techknowlogick
Copy link
Member

🚀

@techknowlogick techknowlogick merged commit 24330f7 into go-gitea:master Nov 23, 2020
@silverwind silverwind deleted the fix-heatmap branch November 24, 2020 14:44
@go-gitea go-gitea locked and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue - heatmap is mini-heatmap
7 participants