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

Apply heatmap coloring to release-download-count #6095

Merged
merged 20 commits into from
Oct 26, 2022

Conversation

foolmoron
Copy link
Contributor

@foolmoron foolmoron commented Oct 21, 2022

Closes #5637

Test URLs

https://github.com/notepad-plus-plus/notepad-plus-plus/releases
https://github.com/notepad-plus-plus/notepad-plus-plus/releases/tag/v8.4.6

Screenshot

image

@fregante
Copy link
Member

fregante commented Oct 21, 2022

I don't see any heat change in your screenshot, they're all almost the same color. The "no heat" color should be more gray, matching what GitHub does. See #5081 (comment) to find out how to use GitHub’s own colors, we shouldn't generate our own.

@foolmoron
Copy link
Contributor Author

The numbers there are all very small other than 1.1M and 460k, so they will share the same heat. I have an idea for how to give more control to the gradient func. I'll try to update the colors too.

@fregante
Copy link
Member

In this case for it to be useful the map should match the minimum and maximum rather than use hardcoded values of what's hot and what's not.

Also we can drop the previous logic to make the hotter download bold.

@fregante
Copy link
Member

fregante commented Oct 21, 2022

Oh, also if they're all low-heat except 1-2 downloads (which is expected), it's fine that they're mostly gray

source/helpers/math.ts Outdated Show resolved Hide resolved
@fregante fregante changed the title Heatmap in release download counts + math func helpers Apply heatmap coloring (like in blame view) to release-download-count Oct 21, 2022
@foolmoron
Copy link
Contributor Author

I don't see any heat change in your screenshot, they're all almost the same color. The "no heat" color should be more gray, matching what GitHub does. See #5081 (comment) to find out how to use GitHub’s own colors, we shouldn't generate our own.

Hm looks like heat colors file isn't loaded on all pages, so we can't use it directly. I can load the file , or copy/paste it with a comment explaining where it comes from.
https://github.githubassets.com/assets/code-d21b20a03009.css

imo copy/paste gives more control and is not much less maintainable.

Co-authored-by: Federico Brigante <me@fregante.com>
@fregante
Copy link
Member

imo copy/paste gives more control and is not much less maintainable.

Sounds fine to me. My only concern was around theming, but it doesn't seem that they're affected by this.

You can just copy the rules that include data-heat like:

.blame-commit[data-heat="1"]{border-right:2px solid #f66a0a}.blame-commit[data-heat="2"]{border-right:2px solid rgba(246,106,10,.9)}.blame-commit[data-heat="3"]{border-right:2px solid rgba(246,106,10,.8)}.blame-commit[data-heat="4"]{border-right:2px solid rgba(246,106,10,.7)}.blame-commit[data-heat="5"]{border-right:2px solid rgba(246,106,10,.6)}.blame-commit[data-heat="6"]{border-right:2px solid rgba(246,106,10,.5)}.blame-commit[data-heat="7"]{border-right:2px solid rgba(246,106,10,.4)}.blame-commit[data-heat="8"]{border-right:2px solid rgba(246,106,10,.3)}.blame-commit[data-heat="9"]{border-right:2px solid rgba(246,106,10,.2)}.blame-commit[data-heat="10"]{border-right:2px solid rgba(246,106,10,.1)}.heat[data-heat="1"]{background:#f66a0a}.heat[data-heat="2"]{background:rgba(246,106,10,.9)}.heat[data-heat="3"]{background:rgba(246,106,10,.8)}.heat[data-heat="4"]{background:rgba(246,106,10,.7)}.heat[data-heat="5"]{background:rgba(246,106,10,.6)}.heat[data-heat="6"]{background:rgba(246,106,10,.5)}.heat[data-heat="7"]{background:rgba(246,106,10,.4)}.heat[data-heat="8"]{background:rgba(246,106,10,.3)}.heat[data-heat="9"]{background:rgba(246,106,10,.2)}.heat[data-heat="10"]{background:rgba(246,106,10,.1)}.blame-commit-date{font-size:12px;line-height:25px;flex-shrink:0}.blame-commit-date[data-heat="1"]{color:#c24e00}.blame-commit-date[data-heat="2"]{color:#ac571f}.blame-commit-date[data-heat="3"]{color:#a35b2c}.blame-commit-date[data-heat="4"]{color:#9a5f38}.blame-commit-date[data-heat="5"]{color:#926245}.blame-commit-date[data-heat="6"]{color:#896651}.blame-commit-date[data-heat="7"]{color:#806a5e}.blame-commit-date[data-heat="8"]{color:#776d6a}.blame-commit-date[data-heat="9"]{color:#6e7177}.blame-commit-date[data-heat="10"]{color:#6a737d}

But the classes need to be changed to .rgh-blah-blah to make sure we don't override their CSS.

@foolmoron
Copy link
Contributor Author

@fregante Alright, updated in latest. Also updated the description and added a new screenshot. wdyt? Looks pretty nice imo

};
}

export function createHeatIndexFunc(values: number[]): (value: number) => number {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe findHeatIndex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a little weird since returns a function, not the head index directly

Copy link
Member

Choose a reason for hiding this comment

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

@fregante do you have a better name for this function?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a reasonable name, other than "Func" which can just be Function. I'm not sure why eslint didn't catch it https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prevent-abbreviations.md

Copy link
Member

Choose a reason for hiding this comment

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

the "lerp" functions should be extended though. Use "linear interpolation" or something else. calculateMinMax can just be inlined as:

const min = Math.min(...values);
const max = Math.max(...values);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes in latest!

source/helpers/math.ts Outdated Show resolved Hide resolved
source/refined-github.css Outdated Show resolved Hide resolved
source/features/release-download-count.tsx Outdated Show resolved Hide resolved
@fregante
Copy link
Member

fregante commented Oct 21, 2022

Starting to look good! I'd probably color the related icon too. Or maybe just the icon. 🤔

I hope it's still all readable. I've been using the colors on the file list but contrast definitely takes a hit. We'll see how people respond.

@foolmoron
Copy link
Contributor Author

Yeah, one option I was thinking is to restrict the heatmap to just the top 3-5 values. So the contrast is largely maintained at the cost of granularity.

Here's the various options of icon vs text coloration. Take your pick!

Just text
image

Just icon
image

Both
image

source/refined-github.ts Outdated Show resolved Hide resolved
source/helpers/math.ts Outdated Show resolved Hide resolved
* @param interpolation Interpolation amount from 0.0 to 1.0 (or beyond to get a value outside the range [min, max])
* @returns Interpolated value
*/
export function linearInterpolation(min: number, max: number, interpolation: number): number {
Copy link
Member

@yakov116 yakov116 Oct 24, 2022

Choose a reason for hiding this comment

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

Why is this function exported?

Better yet where is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I deleted the code that used this. Fixed in latest.

* @param value Value in the range
* @returns Interpolation amount from 0.0 to 1.0 (or beyond if `x` is outside the range [min, max])
*/
export function inverseLinearInterpolation(min: number, max: number, value: number): number {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this function exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in latest

Copy link
Member

Choose a reason for hiding this comment

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

I restored the export because the file is named "math"

@fregante fregante self-assigned this Oct 26, 2022
@fregante
Copy link
Member

I'll work on it 👍

@foolmoron
Copy link
Contributor Author

@fregante I think it should be good now if there's no other comments!

@fregante
Copy link
Member

It was working until I refreshed the page and then all the downloads got a heat of 10 🤔 I'm confused.

@fregante
Copy link
Member

Ok fixed.

.values() returned an iterable which was then empty after the first iteration 😅

@foolmoron
Copy link
Contributor Author

Nice!

@fregante
Copy link
Member

Updating the screenshot in the readme too:

Screen Shot 1

Also mobile screenshot for future reference

Screen Shot 2

@fregante fregante changed the title Apply heatmap coloring (like in blame view) to release-download-count Apply heatmap coloring to release-download-count Oct 26, 2022
@fregante fregante merged commit d99f558 into refined-github:main Oct 26, 2022
@fregante
Copy link
Member

Thank you for the PR, Momin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Apply heatmap coloring (like in blame view) to release-download-count
3 participants