Remove row clicking from notification table

[jolheiser]

Resolves #22692

I don't think there's a need for this entire row to be clickable (and even different links depending on which segment you click)
The links still point to the same spot, so no information is lost here.

[Daeraxa]

I would vote against this as it removes desired behaviour as far as I'm concerned. Not only is row clicking pretty standard UI practice (see github's own) but this is already the behaviour and the reported issue is about missing functionality on top of this. I don't really see justification to having to be pinpoint accurate when looking through a notification list.

[jolheiser]

The alternative, then, would be to make the a take up all of the space, which may require some more tweaking with CSS.

I am fine if that's the consensus, but will need to fiddle around with it. Fomantic tables seem to want to generate padding, so it won't be quite as easy as just making the a take up given space.

[Daeraxa]

The other thought I had is wouldn't this negatively affect the touch/mobile interface as well?

[jolheiser]

The other thought I had is wouldn't this negatively affect the touch/mobile interface as well?

If anything imo it would be better, less chance to misclick, as these table rows actually have different links depending on where in the segment you click.

[jolheiser]

fwiw what "github does" is what I described above, where the a takes up all the space it can.

As well, for things that do the same no matter where you click I think it's fine, but ours is slightly different because you can click to the issue/PR, but there's also a segment for clicking to the repo itself.

So honestly I am fine with either solution, though I don't mind trying to fiddle with the CSS (later) to get it working as-is.

[Daeraxa]

That's a good point, I hadn't noticed that bit. I'm not saying its something that would necessarily need to be adhered to (as GH is far from perfect on the UI front) is that the row click for the GH notifications only seems to stretch as far as it needs to then cuts off when it gets to the "other" links.

Edit: typing this at the same time as you did. Either way I'm just stating a (soft) preference, the main issue I think is inconsistency.

[zeripath]

So I recall making the whole row clickable deliberately to keep things the same as on GH which also did the same thing at that point. Making the <a> take up the whole space seemed to be too difficult.

Now you can still right/middle click on the link bit...

[wxiaoguang]

Interesting.

See #17388 , IMO the click behavior on the td was by purpose before.

If it's not, the JS should be removed together.

[jolheiser]

This is a far cry from making it full-height, but I could not find a "good" way to make it full-height without some very hacky CSS. (admittedly I am not a UI expert by any means)

What GitHub does, and what I suspect we should eventually do (if fomantic lets us), is instead of a table they use a list and cleverly styled divs to render the "table" which allows the anchor tag to encompass the content.

---

I am fine with taking pointers on clever CSS to make this work, accepting this PR as-is, or closing it as "the other way was good enough"

I personally find the dead-space to be a bit tacky, but it does at least bring it closer.

[wxiaoguang]

It will be hacky to make a full height <a>, it needs to remove the td padding and do other works. IMO it's not worth to make it that complex.

The width: 100% should be good enough. And maybe the dead JS code could be removed (if it is only used here) to save browser's CPU time slightly.

[wxiaoguang]

Hmm ... since the HTMLURL is touched, this PR will have conflicts with https://github.com/go-gitea/gitea/pull/21986/files#diff-40b7988fcd35c50d12157e06dc96b586404a4d1d3772a4b257340f11295bec5a

I never understand why people like keeping refactor PRs stale and keeping resolving conflicts.

[lunny]

Please resolve the conflicts.

[wxiaoguang]

Maybe the JS code should also be removed together. Since there won't be <td data-href anymore, the JS code becomes dead code.

gitea/web_src/js/features/common-global.js

Lines 124 to 137 in e7ef94e

	// make table <tr> and <td> elements clickable like a link
	$('tr[data-href], td[data-href]').on('click', function (e) {
	const href = $(this).data('href');
	if (e.target.nodeName === 'A') {
	// if a user clicks on <a>, then the <tr> or <td> should not act as a link.
	return;
	}
	if (e.ctrlKey || e.metaKey) {
	// ctrl+click or meta+click opens a new window in modern browsers
	window.open(href);
	} else {
	window.location = href;
	}
	});

[wxiaoguang]

Is this PR active? I am working on some JS clean-up work, and I could help to propose a sub PR for this PR (to clean the unused JS code).

[jolheiser]

Is this PR active? I am working on some JS clean-up work, and I could help to propose a sub PR for this PR (to clean the unused JS code).

That would be great!
Otherwise I can clean it up later this evening/weekend.

[wxiaoguang]

PR: jolheiser#3

Although it looks like changing a lot, actually only the last commit is related. Other commits are all from "main" branch, because I have to merge the PR's branch with main first, to resolve conflicts.

[jolheiser]

@wxiaoguang looks like I messed something up in my merge, so I reconciled the updating my branch and then cherry-picked your fix commit. Would you mind taking a look to make sure I pulled it correctly and resolved the merge the same as you?

[wxiaoguang]

Nvm. I guess you did a squash merge? IMO a "fast foward" or a normal merge won't cause these messes 😁

I think it looks good to me, thank you very much.

[jolheiser]

Nvm. I guess you did a squash merge? IMO a "fast foward" or a normal merge won't cause these messes 😁

Yep, I used the default without considering it 😓

Thanks for the review!

[wxiaoguang]

ps: I guess line 70: https://github.com/go-gitea/gitea/pull/22695/files#diff-40b7988fcd35c50d12157e06dc96b586404a4d1d3772a4b257340f11295bec5aR70

The <td data-href="{{$repo.Link}}"> could be <td> now?

[codecov-commenter]

Codecov Report

Merging #22695 (dfc6547) into main (f521e88) will increase coverage by 0.00%.
The diff coverage is 41.00%.

❗ Current head dfc6547 differs from pull request most recent head 5662acd. Consider uploading reports for the commit 5662acd to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff            @@
##             main   #22695    +/-   ##
========================================
  Coverage   47.14%   47.15%            
========================================
  Files        1149     1155     +6     
  Lines      151446   152396   +950     
========================================
+ Hits        71397    71855   +458     
- Misses      71611    72069   +458     
- Partials     8438     8472    +34     

Impacted Files	Coverage Δ
cmd/dump.go	0.66% <0.00%> (-0.01%)	⬇️
cmd/web.go	0.00% <0.00%> (ø)
models/actions/run.go	1.64% <0.00%> (-0.08%)	⬇️
models/actions/runner.go	1.44% <ø> (ø)
models/packages/package.go	45.45% <0.00%> (-1.13%)	⬇️
models/user/search.go	77.50% <0.00%> (-6.29%)	⬇️
modules/actions/workflows.go	0.00% <0.00%> (ø)
modules/context/context.go	64.54% <0.00%> (-3.53%)	⬇️
modules/doctor/storage.go	30.65% <0.00%> (-1.29%)	⬇️
modules/setting/git.go	45.45% <ø> (ø)
... and 36 more

... and 45 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[silverwind]

This does also fix the padding issue on empty list. I think generally this list should be reworked to no longer be a <table>, but that can be done later.