add github like contribution graph css and ceil for 1,2 report count #453

kelvin2go · 2021-10-06T09:41:30Z

math.ceil for 1,2
add github contribution graph like css

kelvin2go · 2021-10-07T06:49:29Z

adaptive max contribution
max(10, max(user_contribution)) 作為 contribution graph 的 max scale

for this, should it changes max scale to 10 ?

https://github.com/cofacts/rumors-site/pull/453/files#diff-b170cff4fb01eac4d00fe240d828301e6e8900a7ae3443ad833eeff2a6d64d8fR13

and for test part should i update the story snapshot after implement?

@MrOrz any suggestion :)

MrOrz · 2021-10-10T05:35:04Z

Sorry for the late reply, and thanks for taking this issue!

Current coloring of attribution is fixed at 5 contributions per level (SCALING_FACTOR). Therefore, daily contribution must exceed 23 (23 / SCALING_FACTOR = 4.6 ~= 5) to be painted to deepest color.
The adaptive max contribution proposal wants that max scale to adapt to each user. For example, if the max contribution count of the user is 16 for the past 365 days, we should color the contribution count 1 ~ 4 with light color, 5 ~ 8 with deeper color, 9 ~ 12 with 2nd deeper color, and 13 ~ 16 with deepest color.
However, if the max contribution count of the user for the past 365 days is lower than 10, it would be a bit unfair for other contributors if we also paint those contribution as deepest color. Therefore, in the proposal we added a max-contribution lower-bound: if the max contribution count of the user for the past 365 days is lower than 10, we will use 10 when calculating the color scale.

If we put it in code,

MAX_SCALE remains 4, as available colorCofactsN still ranges from colorCofacts0 to colorCofacts4.
SCALING_FACTOR (number of contributions needed to go to the deeper color) should no longer be a constant across different users. If max contribution in data is below 10, the scaling factor is 10/MAX_SCALE = 2.5; otherwise, it is max contribution / MAX_SCALE.
The logic of scaleColor is Math.max(Math.min(Math.ceil(count / varingScalingFactor), MAX_SCALE), 0). The inner-most Math.ceil is identical to your current PR 👍

And yes, the storyshot should be updated and attached to the PR, so that we can know there are DOM changes in the PR 👀

…e/413-contribution-graph-enhancements

kelvin2go · 2021-10-15T07:27:54Z

@MrOrz I updated varingScalingFactor and the snapshot. Can you check it ? but snapshot test seem fail on other component.

        modified:   components/AppLayout/AppHeader.js
        modified:   components/AppLayout/AppSidebar.js
        modified:   components/AppLayout/__snapshots__/UpgradeDialog.stories.storyshot
        modified:   components/ArticleCategories/CategoryOption.js
        modified:   components/ArticleReplyFeedbackControl/__snapshots__/ArticleReplyFeedbackControl.stories.storyshot
        modified:   components/Infos/__snapshots__/Infos.stories.storyshot
        modified:   components/ListPageControls/__snapshots__/BaseSortInput.stories.storyshot
        modified:   components/ListPageControls/__snapshots__/BaseTimeRange.stories.storyshot
        modified:   components/ListPageControls/__snapshots__/Filters.stories.storyshot
        modified:   components/ListPageControls/__snapshots__/LoadMore.stories.storyshot
        modified:   components/ProfilePage/UserPageHeader.js
        modified:   components/ReportPage/__snapshots__/ActionButton.stories.storyshot
        modified:   components/__snapshots__/Hyperlinks.stories.storyshot
        modified:   components/__snapshots__/TrendPlot.stories.storyshot
        modified:   components/__snapshots__/icons.stories.storyshot

nonumpa · 2021-10-16T06:27:18Z

@kelvin2go It seems that your local material-ui version is different from this project.

You can try the command writing in README.md :

# install the same npm package as this project
npm install

# run test and update snapshot
npm test -- -u

kelvin2go · 2021-10-16T06:51:03Z

@kelvin2go It seems that your local material-ui version is different from this project.

You can try the command writing in README.md :
# install the same npm package as this project
npm install

# run test and update snapshot
npm test -- -u

hi @nonumpa,
i only changed components/ContributionChart.js
so i update only
components/snapshots/ContributionChart.stories.storyshot
or when i run test -- -u it changed other files as below. shoud i updated it to branch as well?

    modified:   components/AppLayout/__snapshots__/UpgradeDialog.stories.storyshot
        modified:   components/ArticleReplyFeedbackControl/__snapshots__/ArticleReplyFeedbackControl.stories.storyshot
        modified:   components/Infos/__snapshots__/Infos.stories.storyshot
        modified:   components/ListPageControls/__snapshots__/BaseSortInput.stories.storyshot
        modified:   components/ListPageControls/__snapshots__/BaseTimeRange.stories.storyshot
        modified:   components/ListPageControls/__snapshots__/Filters.stories.storyshot
        modified:   components/ListPageControls/__snapshots__/LoadMore.stories.storyshot
        modified:   components/ReportPage/__snapshots__/ActionButton.stories.storyshot
        modified:   components/__snapshots__/Hyperlinks.stories.storyshot
        modified:   components/__snapshots__/TrendPlot.stories.storyshot
        modified:   components/__snapshots__/icons.stories.storyshot

nonumpa · 2021-10-16T07:26:41Z

It's abnormal, and I guess other .storyshot files also change className="makeStyles-tooltipText-311" to className="makeStyles-tooltipText-74".

Maybe you can google the keyword "npm local vs global package" to solve this problem.

Explanation of why I guess your local material-ui version is different from this project and why it should not have className="makeStyles-tooltipText-*" change :

First, we take a look at the .storyshot change

And then look what CI test result says

line 35 and 36 means
It should be aria-hidden="true" not aria-hidden={true}

line 48 and 49 means
It should be className="makeStyles-tooltipText-311" not className="makeStyles-tooltipText-74"

That means only className="makeStyles-colorCofacts*" is the right change.
This makes sense because you only change the function scaleColor which only affects colorCofacts*.

MrOrz · 2021-10-16T12:26:34Z

No worries, I think I can make a commit that tries to fix CI 💪

kelvin2go · 2021-10-16T13:58:01Z

hi @MrOrz ,
after test with @nonumpa
look like "@material-ui/core": "^4.9.7",

4.9.9 can pass the test.

not sure, which version is CI using @@'

coveralls · 2021-10-16T14:33:16Z

Coverage increased (+0.03%) to 75.294% when pulling 157a563 on kelvin2go:feature/413-contribution-graph-enhancements into 8c26bbb on cofacts:master.

MrOrz

Thanks for trying to fix this issue! An implementation detail of color scale is attached below.

components/ContributionChart.js

johnson-liang

I will make a commit that fix the bug below

johnson-liang · 2021-10-20T04:48:43Z

components/ContributionChart.js

-  return Math.max(Math.min(Math.round(count / SCALING_FACTOR), MAX_SCALE), 0);
+function scaleColor(count, maxContribution) {
+  const varingScalingFactor =
+    count < 10 ? 10 / MAX_SCALE : maxContribution.count / MAX_SCALE;


Should be

Suggested change

count < 10 ? 10 / MAX_SCALE : maxContribution.count / MAX_SCALE;

maxContribution.count < 10 ? 10 / MAX_SCALE : maxContribution.count / MAX_SCALE;

Current logic will cause count < 10 to have deeper color than count > 10.

MrOrz

Now the new color system and mobile style is working as expected. Thank you for the fix!

add github like contribution graph css and ceil for 1,2 report count

a748f12

nonumpa requested a review from MrOrz October 9, 2021 02:03

MrOrz linked an issue Oct 10, 2021 that may be closed by this pull request

Contribution graph enhancements #413

Closed

kelvin2go added 3 commits October 15, 2021 14:33

Merge branch 'master' of github.com:kelvin2go/rumors-site into featur…

4aff910

…e/413-contribution-graph-enhancements

add varingScalingFactor

d392b0f

add snapshots

79e7120

fix snapshot with material-ui

28be3e2

Fix storyshot

c650642

MrOrz requested changes Oct 16, 2021

View reviewed changes

components/ContributionChart.js Outdated Show resolved Hide resolved

add corrected MaxContribution

a1d799b

johnson-liang reviewed Oct 20, 2021

View reviewed changes

Fix scale color in ContributionChart

157a563

MrOrz approved these changes Oct 20, 2021

View reviewed changes

MrOrz merged commit 3b5ad97 into cofacts:master Oct 20, 2021

MrOrz mentioned this pull request Oct 21, 2021

Adjust ContributionChart coloring #454

Merged

kelvin2go deleted the feature/413-contribution-graph-enhancements branch October 21, 2021 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add github like contribution graph css and ceil for 1,2 report count #453

add github like contribution graph css and ceil for 1,2 report count #453

kelvin2go commented Oct 6, 2021

kelvin2go commented Oct 7, 2021 •

edited

Loading

MrOrz commented Oct 10, 2021 •

edited

Loading

kelvin2go commented Oct 15, 2021 •

edited

Loading

nonumpa commented Oct 16, 2021 •

edited

Loading

kelvin2go commented Oct 16, 2021

nonumpa commented Oct 16, 2021

MrOrz commented Oct 16, 2021 •

edited

Loading

kelvin2go commented Oct 16, 2021

coveralls commented Oct 16, 2021 •

edited

Loading

MrOrz left a comment

johnson-liang left a comment

johnson-liang Oct 20, 2021

MrOrz left a comment

	count < 10 ? 10 / MAX_SCALE : maxContribution.count / MAX_SCALE;
	maxContribution.count < 10 ? 10 / MAX_SCALE : maxContribution.count / MAX_SCALE;

add github like contribution graph css and ceil for 1,2 report count #453

add github like contribution graph css and ceil for 1,2 report count #453

Conversation

kelvin2go commented Oct 6, 2021

kelvin2go commented Oct 7, 2021 • edited Loading

MrOrz commented Oct 10, 2021 • edited Loading

kelvin2go commented Oct 15, 2021 • edited Loading

nonumpa commented Oct 16, 2021 • edited Loading

kelvin2go commented Oct 16, 2021

nonumpa commented Oct 16, 2021

MrOrz commented Oct 16, 2021 • edited Loading

kelvin2go commented Oct 16, 2021

coveralls commented Oct 16, 2021 • edited Loading

MrOrz left a comment

Choose a reason for hiding this comment

johnson-liang left a comment

Choose a reason for hiding this comment

johnson-liang Oct 20, 2021

Choose a reason for hiding this comment

MrOrz left a comment

Choose a reason for hiding this comment

kelvin2go commented Oct 7, 2021 •

edited

Loading

MrOrz commented Oct 10, 2021 •

edited

Loading

kelvin2go commented Oct 15, 2021 •

edited

Loading

nonumpa commented Oct 16, 2021 •

edited

Loading

MrOrz commented Oct 16, 2021 •

edited

Loading

coveralls commented Oct 16, 2021 •

edited

Loading