Heatmap days clickable #13935

LFriede · 2020-12-10T22:08:48Z

Hi,

I filed an issue (#10843) about making the days in the heatmap clickable like here on github to display the actions on a specific day. I've implemented it with this PR.

techknowlogick · 2020-12-10T22:20:06Z

Could you provide a screenshot/gif of this PR in action?

codecov-io · 2020-12-10T23:00:28Z

Codecov Report

Merging #13935 (9f31f44) into master (487f2ee) will decrease coverage by 0.00%.
The diff coverage is 42.15%.

@@            Coverage Diff             @@
##           master   #13935      +/-   ##
==========================================
- Coverage   42.21%   42.20%   -0.01%     
==========================================
  Files         767      771       +4     
  Lines       81624    82078     +454     
==========================================
+ Hits        34458    34644     +186     
- Misses      41531    41784     +253     
- Partials     5635     5650      +15

Impacted Files	Coverage Δ
models/action.go	`48.52% <0.00%> (-2.42%)`	⬇️
models/migrations/migrations.go	`2.59% <ø> (ø)`
models/migrations/v166.go	`0.00% <0.00%> (ø)`
models/migrations/v172.go	`0.00% <0.00%> (ø)`
models/migrations/v173.go	`0.00% <0.00%> (ø)`
models/session.go	`0.00% <0.00%> (ø)`
models/user.go	`53.05% <ø> (+0.38%)`	⬆️
modules/context/context.go	`58.05% <0.00%> (-0.45%)`	⬇️
modules/indexer/code/elastic_search.go	`1.72% <0.00%> (-0.02%)`	⬇️
modules/migrations/github.go	`77.77% <0.00%> (-1.99%)`	⬇️
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3e64f6...9f31f44. Read the comment docs.

silverwind · 2020-12-11T18:01:20Z

Can you make it clear the filter when clicking the same date again?

silverwind · 2020-12-11T18:03:08Z

models/action.go

+		var dateLow time.Time
+		var dateHigh time.Time
+
+		dateLow, _ = time.Parse("2006-1-2", opts.Date)


Shouldn't ignore the error.

Suggested change

dateLow, _ = time.Parse("2006-1-2", opts.Date)

var err error

dateLow, err = time.Parse("2006-1-2", opts.Date)

if err != nil {

return fmt.Errorf("Unable to parse %s: %v", opts.Date, err)

}

should do it.

I think we have two ways to handle the error. The way @zeripath suggested or by just logging the error. Returning the error will cause a 500 error code. Logging the error seems to be closer to the behavior on other parts of gitea. For example if I use invalid parameters for sorting the issue list it just don't sort instead of throwing an error.

I'm not sure which way is better. What do you think?

dateLow, err := time.Parse("2006-1-2", opts.Date) if err != nil { log.Warn("Unable to parse %s, filter not applied: %v", opts.Date, err) } else { dateHigh := dateLow.Add(86399000000000) // 23h59m59s cond = cond.And(builder.Gte{"created_unix": dateLow.Unix()}) cond = cond.And(builder.Lte{"created_unix": dateHigh.Unix()}) }

I like yours because if we fail to parse the date, there is no reason to continue as the functions below would fail as well (would that be a panic?).

LFriede · 2020-12-11T19:45:39Z

I'll do a gif, filter clearing by clicking again, and error checking as soon as possible :)

… heatmap_click

LFriede · 2020-12-15T20:07:09Z

I pushed the changes and here is a gif.

silverwind · 2020-12-15T21:43:47Z

Please apply this diff for more robust JS:

diff --git a/web_src/js/components/ActivityHeatmap.vue b/web_src/js/components/ActivityHeatmap.vue
index c06fd8799..b9a3aa3a6 100644
--- a/web_src/js/components/ActivityHeatmap.vue
+++ b/web_src/js/components/ActivityHeatmap.vue
@@ -41,21 +41,22 @@ export default {
       no_contributions: 'No contributions',
     },
   }),
   methods: {
-    handleDayClick(event) {
+    handleDayClick(e) {
       // Reset filter if same date is clicked
-      const day = window.location.search.match(/\?date=(\d{4})-(\d{1,2})-(\d{1,2})/);
-      if (day !== null) {
-        if (day.length === 4) {
-          if ((parseInt(day[1]) === event.date.getFullYear()) && (parseInt(day[2]) === (event.date.getMonth() + 1)) && (parseInt(day[3]) === event.date.getDate())) {
-            window.location.search = '';
-            return;
-          }
-        }
+      const params = new URLSearchParams(document.location.search);
+      const queryDate = params.get('date');
+      const clickedDate = e.date.toISOString().substring(0, 10);

+      if (queryDate && queryDate === clickedDate) {
+        params.delete('date');
+      } else {
+        params.set('date', clickedDate);
       }

-      window.location.search = `?date=${event.date.getFullYear()}-${event.date.getMonth() + 1}-${event.date.getDate()}`;
+      const newSearch = params.toString();
+      window.location.search = newSearch.length ? `?${newSearch}` : '';
     }
   },
 };
 </script>

LFriede · 2020-12-19T13:20:04Z

Honestly I didn't knew the URLSearchParams object, thats the better solution of course.

Your date comparison trades 2020-2-1 and 2020-02-01 as different dates, thats fine as long as nobody tries to edit the url by hand. The regex approach handles that case (on purpose). So I think we should combine both to something like this:

    handleDayClick(e) {
      // Reset filter if same date is clicked
      const params = new URLSearchParams(document.location.search);
      const queryDate = params.get('date');

      let sameDate = false;
      if (queryDate !== null) {
        const day = queryDate.match(/(\d{4})-(\d{1,2})-(\d{1,2})/);
        if (day !== null) {
          if (day.length === 4) {
            if ((parseInt(day[1]) === e.date.getFullYear()) && (parseInt(day[2]) === (e.date.getMonth() + 1)) && (parseInt(day[3]) === e.date.getDate())) {
              sameDate = true;
            }
          }
        }
      }

      if (sameDate) {
        params.delete('date');
      } else {
        params.set('date', `${e.date.getFullYear()}-${e.date.getMonth() + 1}-${e.date.getDate()}`);
      }

      const newSearch = params.toString();
      window.location.search = newSearch.length ? `?${newSearch}` : '';
    }

Do you agree?

silverwind · 2020-12-19T16:57:57Z

No, I'd enforce YYYY-MM-DD, it's a well-accepted standard format for a date, no need to support anything else.

LFriede · 2020-12-19T18:09:50Z

Okay, I pushed your version but with a small correction. Timezone is substracted from the date because at least in Germany (UTC+2) it returns one day off since toISOString() formats the date to UTC+0.

noerw · 2020-12-21T08:05:06Z

routers/user/home.go

@@ -159,6 +159,7 @@ func Dashboard(ctx *context.Context) {
 		IncludePrivate:  true,
 		OnlyPerformedBy: false,
 		IncludeDeleted:  false,
+		Date:            ctx.Query("date"),


You're missing this parameter in routers/user/profile.go.

noerw · 2020-12-21T08:05:30Z

models/action.go

+	IncludePrivate  bool   // include private actions
+	OnlyPerformedBy bool   // only actions performed by requested user
+	IncludeDeleted  bool   // include deleted actions
+	Date            string // the day we want activity for: YYYY-M-D


Would you mind changing the signature of GetFeedsOptions to not specify a single date, but to contain a time range (i.e. after, before time.Time, populate these values in home.go/profile.go)? In #14080 I'm making this struct reusable for the heatmap, and a time-range filter would be welcome for the heatmap too (for #5439).

models/action.go

6543 · 2021-02-02T17:30:49Z

@LFriede would be nice if you can resolve conflicts :)

LFriede · 2021-02-03T21:06:31Z

Sorry for the late reply.

Added forgotten date parameter on profile.go like @noerw pointed out. I'd prefer to add date range in another PR if thats okay.
Edited parsed date format like @silverwind said.
resolved conflicts

noerw

looks good, please merge current master

LFriede and others added 2 commits December 10, 2020 23:03

Heatmap days clickable

1faf528

Merge branch 'master' into heatmap_click

3511854

techknowlogick added the topic/ui Change the appearance of the Gitea UI label Dec 10, 2020

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 10, 2020

6543 added the type/enhancement An improvement of existing functionality label Dec 11, 2020

Merge branch 'master' into heatmap_click

0474af9

6543 approved these changes Dec 11, 2020

View reviewed changes

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 Dec 11, 2020

6543 added this to the 1.14.0 milestone Dec 11, 2020

silverwind reviewed Dec 11, 2020

View reviewed changes

LFriede added 3 commits December 15, 2020 20:56

Error handling

c7709a9

Unselect filter

058602d

Merge branch 'heatmap_click' of https://github.com/LFriede/gitea into…

d73822c

… heatmap_click

better dayclick handler

23a5034

noerw suggested changes Dec 21, 2020

View reviewed changes

silverwind reviewed Dec 22, 2020

View reviewed changes

models/action.go Outdated Show resolved Hide resolved

LFriede added 3 commits February 3, 2021 21:38

fixed conflicts

70829b1

made linter happy

e022404

clickable heatmap for profiles

8656ad3

6543 added 2 commits February 3, 2021 22:37

Merge branch 'master' into heatmap_click

33c4823

Merge branch 'master' into heatmap_click

493004a

noerw approved these changes Feb 20, 2021

View reviewed changes

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 Feb 20, 2021

Merge branch 'master' into heatmap_click

9f31f44

techknowlogick merged commit 343c756 into go-gitea:master Feb 20, 2021

LFriede mentioned this pull request Feb 21, 2021

Heatmap squares clickable #10843

Closed

7 tasks

noerw mentioned this pull request Feb 25, 2021

add daily specific Contribution Activity #14796

Closed

mayswind mentioned this pull request Mar 25, 2021

Fix timezone bug when clicking heatmap #15141

Merged

go-gitea locked and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heatmap days clickable #13935

Heatmap days clickable #13935

LFriede commented Dec 10, 2020

techknowlogick commented Dec 10, 2020

codecov-io commented Dec 10, 2020 •

edited

silverwind commented Dec 11, 2020

silverwind Dec 11, 2020

zeripath Dec 11, 2020

LFriede Dec 11, 2020 •

edited

silverwind Dec 11, 2020 •

edited

LFriede commented Dec 11, 2020

LFriede commented Dec 15, 2020

silverwind commented Dec 15, 2020

LFriede commented Dec 19, 2020

silverwind commented Dec 19, 2020

LFriede commented Dec 19, 2020

noerw Dec 21, 2020

noerw Dec 21, 2020

6543 commented Feb 2, 2021

LFriede commented Feb 3, 2021

noerw left a comment

-		dateLow, _ = time.Parse("2006-1-2", opts.Date)
+		var err error
+		dateLow, err = time.Parse("2006-1-2", opts.Date)
+		if err != nil {
+			return fmt.Errorf("Unable to parse %s: %v", opts.Date, err)
+		}

Heatmap days clickable #13935

Heatmap days clickable #13935

Conversation

LFriede commented Dec 10, 2020

techknowlogick commented Dec 10, 2020

codecov-io commented Dec 10, 2020 • edited

Codecov Report

silverwind commented Dec 11, 2020

silverwind Dec 11, 2020

Choose a reason for hiding this comment

zeripath Dec 11, 2020

Choose a reason for hiding this comment

LFriede Dec 11, 2020 • edited

Choose a reason for hiding this comment

silverwind Dec 11, 2020 • edited

Choose a reason for hiding this comment

LFriede commented Dec 11, 2020

LFriede commented Dec 15, 2020

silverwind commented Dec 15, 2020

LFriede commented Dec 19, 2020

silverwind commented Dec 19, 2020

LFriede commented Dec 19, 2020

noerw Dec 21, 2020

Choose a reason for hiding this comment

noerw Dec 21, 2020

Choose a reason for hiding this comment

6543 commented Feb 2, 2021

LFriede commented Feb 3, 2021

noerw left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 10, 2020 •

edited

LFriede Dec 11, 2020 •

edited

silverwind Dec 11, 2020 •

edited