Skip to content

Heatmap mini fix #13637

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Heatmap mini fix #13637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3cb6c58
fix: show dashboard heatmap in full size
wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf Nov 19, 2020
4383c2f
fix: heatmap not showing up below 1200w
wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf Nov 19, 2020
046d8c8
chore: nice heatmap alignment with display: inline
wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf Nov 19, 2020
474119d
chore: keep error message centering
wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf Nov 19, 2020
f2472b0
lint: fix error when using double quotes
wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf Nov 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions web_src/js/features/heatmap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export default async function initHeatmap() {
new View().$mount(el);
} catch (err) {
console.error(err);
el.style.display = 'flex';
el.textContent = 'Heatmap failed to load';
}
}
9 changes: 1 addition & 8 deletions web_src/less/features/heatmap.less
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
#user-heatmap {
width: 100%;
display: inline;
text-align: center;
position: relative;
min-height: 125px;
display: flex;
align-items: center;
justify-content: center;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

gitea/web_src/js/features/heatmap.js

Line 13 in ad2a288

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@silverwind silverwind Nov 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

the error message could be flex-centered

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Not sure yet, need to test later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


Expand All @@ -16,13 +16,6 @@
fill: currentColor !important;
}

@media @mediaLgAndDown {
&,
& + .divider {
display: none;
}
}

.total-contributions {
font-size: 11px;
position: absolute;
Expand Down