-
Notifications
You must be signed in to change notification settings - Fork 942
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
Add user profile heatmap visualization for contributions #5402
base: master
Are you sure you want to change the base?
Conversation
b27b7a6
to
b134a0a
Compare
Oh, I forgot about this. |
It is a very minor nitpick, but is it possible to dynamically determine what's the first weekday based on the user's locale and adjust the heatmap accordingly? Intl.Locale.prototype.getWeekInfo() could be used. It isn't currently available in Firefox but all other browsers have it. P.S. Added bug 1937867 to Firefox's bug tracker. |
What's the source(s) of all the javascript in Also, how does the inclusion of popper interact with the popper already included? We end up with |
OK, I found the answer for the cal-heatmap part of the code, which is available as an npm module. In which case, using package.json will allow us to upgrade to new versions automatically, and would be the preferred option. |
UX thoughts:
Code comments:
|
I've tested the performance of the underlying query and using the user with the most changesets in the last year (just over 50 thousand) it took a fraction over one second which is probably acceptable. That result (which was a full 365 days of data) serialised to just under 10Kb which is a fair chunk to be caching but hopefully most entries will be smaller than that. |
Thank you for the suggestion. I’ll refactor accordingly to ensure alignment with the preferred color scheme setting.
I tried dynamically adjusting the start and end weekdays using ghDay as the subdomain, but it rounds to the first and last weeks of the month, making implementation tricky. Accommodating this would require significant effort, which may not be worth it for such a minor adjustment.
To be honest I wasn’t too familiar with how our assets pipeline works,so I just added the Thank you for pointing out the existing |
0a8fc14
to
af02b6a
Compare
I've updated the code to account for the "Preferred Website Color Scheme." Theme settings should now be working correctly. UX Thoughts:
Code Comments:
Also a big thank you to @tomhughes for testing the performance here. |
How is it included in |
It's not going to work correctly because our Auto option follows Looks like cal-heatmap doesn't have a proper auto scheme "as not all websites support dark/light mode". You'd have to then setup a listener like this but without using Leaflet of course and redraw the heatmap. Same is true for #5426 by the way. |
Any other change to the theme of the page without reloading will break most of the CSS theming anyway since the stylesheets were split in #5362 by the way. It would be great to have a global leaflet-independent watcher where clients can add themselves to a callback list. |
You don't need to change the theme of the page, neither here, nor in #5426. Or do you say that you haven't tried switching
Leaflet code is just a wrapper for |
I did that for testing, but since listening increased the complexity unnecessarily, I left that for a second pass. But suppose the site needs a theme watcher anyway because the cal-heatmap doesn't want to let CSS do the color mixing theme-agnostically. In that case, instead of having two listeners, a common solution that includes MutationObservers outside of jQuery would be the one with the least overhead. |
Why MutationObservers? |
The MutationObserver thought came from my tinkering with the site locally wanting to reduce the reloads, forgetting that a toggle if added would already have onchange events.
Yeah, I kinda missed that... Still, having a similar style on such similar functions and having them moved to some unified location would be a nice thing to have, currently, they could be much more different. But that's more of a refactor thing if both are merged. |
940feb9
to
8028378
Compare
I've moved the import to
Added the logic to handle the auto prefered-color-changes. Should be resolved now as well.
I'm working on resolving this CSP violations. Right now I'm trying to generate nonce for the .js file and use that to validate and transpass the CSP violation. I'm not sure if it is the right approach for this since I never had experience dealing with similar stuff before. If there is a more elegant and compact solution, please feel free to point it out. Thank you. |
For the wrong one though. Another copy of the html's bs-theme attribute isn't necessarily helpful, and the media query is still not being listened to. |
app/assets/javascripts/heatmap.js
Outdated
function getThemeFromColorScheme(colorScheme) { | ||
if (colorScheme === "auto") { | ||
return window.matchMedia("(prefers-color-scheme: dark)").matches ? "dark" : "light"; | ||
} | ||
return colorScheme; // Return "light" or "dark" directly if specified | ||
} |
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.
I'm sorry, but I don't see where a re-draw would be triggered.
|
e1bc1b9
to
ccaeee9
Compare
ec5f4a5
to
341fe13
Compare
a068ef7
to
38f9093
Compare
ac4ecca
to
f9d674f
Compare
After another checks re-run, all tests are passing as expected. |
f9d674f
to
1f4672c
Compare
yarn.lock
Outdated
@@ -4,24 +4,24 @@ | |||
|
|||
"@aashutoshrathi/word-wrap@^1.2.3": | |||
version "1.2.6" | |||
resolved "https://registry.yarnpkg.com/@aashutoshrathi/word-wrap/-/word-wrap-1.2.6.tgz#bd9154aec9983f77b3a034ecaa015c2e4201f6cf" | |||
resolved "https://registry.npmjs.org/@aashutoshrathi/word-wrap/-/word-wrap-1.2.6.tgz" |
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.
Now there's a ton of registry.yarnpkg.com
-> registry.npmjs.org
changes.
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.
Must have messed this up during conflict. I will try to restore the file and do rebase again. Thank you.
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.
Now there are no updates to yarn.lock
.
d2e9531
to
edeaaa5
Compare
edeaaa5
to
c10ac0d
Compare
Description
This PR addresses #5373 by adding a heatmap visualization on user profile pages to display daily contribution activity over the past year.
Key changes include:
users/show.html.erb
).heatmap.js
) using CalHeatmap to render the heatmap dynamically based on contribution data.config/locales/en.yml
) to include tooltips and month/weekday labels for the heatmap.users_controller_test.rb
to validate heatmap data handling.Screenshots:
![Screenshot 2024-12-16 at 16 35 58](https://private-user-images.githubusercontent.com/76796906/396157838-878068ee-c0cd-450d-b3e8-010a9c75575f.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk1NzYzOTgsIm5iZiI6MTczOTU3NjA5OCwicGF0aCI6Ii83Njc5NjkwNi8zOTYxNTc4MzgtODc4MDY4ZWUtYzBjZC00NTBkLWIzZTgtMDEwYTljNzU1NzVmLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE0VDIzMzQ1OFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWE0Zjk3MTMwY2NlODlkOTRjZTFiY2JkMWQ3NmIwMGI2MzBlOWIwNzg0NzY2OGNjNzUzYmFjODRiOTc1NzFjMzUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.3AXOVNhAQpmI87DMwlzT4BpdWJ6tmFp027BDtJZjQFE)
![Screenshot 2024-12-16 at 16 37 14](https://private-user-images.githubusercontent.com/76796906/396158347-2ae8ff02-0416-41fe-aeeb-d91cc6584a67.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk1NzYzOTgsIm5iZiI6MTczOTU3NjA5OCwicGF0aCI6Ii83Njc5NjkwNi8zOTYxNTgzNDctMmFlOGZmMDItMDQxNi00MWZlLWFlZWItZDkxY2M2NTg0YTY3LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE0VDIzMzQ1OFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTY3YzVkZjk5MDg0MjE3NWE0NDk5Y2EwYjM2NmUwZmZkNzI0M2Q4ZDg0NmE3MDZmODIzN2U2ZDg5MzNjZDkxNjQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.ZJHHUpEHFHFVkd2E1ef_y89FGZmROglvobl1rW6aTlc)
![Screenshot 2024-12-16 at 16 36 25](https://private-user-images.githubusercontent.com/76796906/396158017-ca4d4b80-cdbf-4b20-a5e6-602ef3ec07ce.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk1NzYzOTgsIm5iZiI6MTczOTU3NjA5OCwicGF0aCI6Ii83Njc5NjkwNi8zOTYxNTgwMTctY2E0ZDRiODAtY2RiZi00YjIwLWE1ZTYtNjAyZWYzZWMwN2NlLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE0VDIzMzQ1OFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTg0ZDcwOTdjYjBkMGE4ZTFiMmM3MDQzNTYwOWY5MmU4NGJjZWNkYjQ3ZTAzODk1OGY5ZDdkN2JiMDk4NTRlNjEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.7swCerLaqjdNACwvkCajCi2ddL2BiCtr5_q6r_1_-N0)
How has this been tested?
Let me know if you need further tweaks or enhancements!