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

Restore edit-comments-faster on review comments #3407

Merged
merged 12 commits into from
Oct 7, 2020

Conversation

kidonng
Copy link
Member

@kidonng kidonng commented Jul 27, 2020

Maybe a continuation of #2992

Obsolete information

Explanation of the lazy load logic mentioned in #3396

.js-minimizable-comment-group
	.minimized-comment // Invisible
		...
			details-menu
				.js-comment-edit-button // A
    .js-comment
		...
			details-menu[src]
				// Contents are lazy loaded when you hover the three dots menu
				.js-comment-edit-button // B
  1. Why Up arrow to edit own comment is broken #3388 happens: comment-fields-keyboard-shortcuts tries to find B which isn't loaded yet
  2. Why edit-comments-faster in review comments doesn't work: only A triggers the function and the icon is added into an invisible container
  3. Why Up arrow to edit own comment is broken #3388 (comment) happens: B loads and triggers the function and the icon is added into an invisible container

This PR

  • Determine if it's A triggering the function, and add the edit icon to the correct location in review comments (fix 2)
  • Skip if it's B triggering the function, to prevent duplicate icon addition (fix 3)
  • Upgrade to selector-observer, fix "Edit comment" button injection is not reliable #3279
  • Rewrite the feature to fix existing issues

Test: A PR with review comments (like https://github.com/sindresorhus/refined-github/pull/3405#discussion_r460863841)

@fregante
Copy link
Member

fregante commented Jul 27, 2020

This seems unnecessary. As I suggested earlier, the solution is to create a brand-new button instead of cloning the existing one. This way you just need to select the emoji dropdown and avoid all this document traversing/filtering.

@fregante fregante added the bug label Jul 27, 2020
@fregante fregante marked this pull request as draft July 27, 2020 19:05
@kidonng kidonng marked this pull request as ready for review July 27, 2020 21:06
@kidonng
Copy link
Member Author

kidonng commented Jul 27, 2020

Filtering is needed because there can be two edit buttons in a single review comment.

Check out the latest code, maybe the simplest one.

@fregante fregante marked this pull request as draft July 29, 2020 16:12
@fregante
Copy link
Member

fregante commented Jul 29, 2020

Please always provide a link where this is testable, as requested in the PR template.

And what it fixes exactly

@kidonng
Copy link
Member Author

kidonng commented Jul 29, 2020

Sorry, added test link and make what this PR does more clear.

I just found there are still rooms for improvement: currently it doesn't work if you unresolve a conversation

@kidonng
Copy link
Member Author

kidonng commented Aug 2, 2020

Seems like GitHub removed the aforementioned lazy load logic, so this bug doesn't exist anymore. Closing for the moment.

Update on 2020-08-24: it is back 😥

@kidonng kidonng closed this Aug 2, 2020
@kidonng kidonng deleted the edit-comments-faster branch August 9, 2020 15:15
@kidonng kidonng restored the edit-comments-faster branch August 24, 2020 11:53
@kidonng kidonng reopened this Aug 24, 2020
@kidonng kidonng marked this pull request as ready for review August 28, 2020 07:59
@kidonng
Copy link
Member Author

kidonng commented Aug 28, 2020

After using selector-observer it works for unresolved conversations 🎉

@fregante
Copy link
Member

I'm still seeing the lazy-loaded context-menus. For the 3rd time, do not use cloneNode. Just generate the link via JSX.

@fregante fregante marked this pull request as draft August 28, 2020 08:02
@kidonng
Copy link
Member Author

kidonng commented Aug 28, 2020

I'm still seeing the lazy-loaded context-menus

That's why I reopened this PR (though it now also includes upgrading to selector-observer)

For the 3rd time, do not use cloneNode

🤷‍♀️ That cloneNode is not written by me

@fregante fregante marked this pull request as ready for review August 28, 2020 08:08
@fregante fregante changed the title Fix edit-comments-faster in review comments Fix edit-comments-faster for review comments Aug 29, 2020
Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Thank you!

Note: hasComment automatically implies a comment event listener though. This has no effect since we use onetime, but we should keep it in mind.

Perhaps we can remove that implication once we refactor every/most hasComments feature to use selector-observer

@fregante fregante closed this Sep 9, 2020
@fregante fregante reopened this Sep 25, 2020
@fregante fregante marked this pull request as draft September 25, 2020 01:15
@kidonng kidonng marked this pull request as ready for review October 7, 2020 07:30
@kidonng
Copy link
Member Author

kidonng commented Oct 7, 2020

I reverted 9c147cd with new selectors. Tested on other repos where I have permissions, the edit button appears on where it should.

@kidonng kidonng requested a review from fregante October 7, 2020 14:36
@fregante fregante marked this pull request as draft October 7, 2020 21:57
@fregante fregante closed this Oct 7, 2020
@fregante fregante self-assigned this Oct 7, 2020
@fregante fregante reopened this Oct 7, 2020
@fregante fregante removed their assignment Oct 7, 2020
@fregante fregante marked this pull request as ready for review October 7, 2020 22:30
@fregante fregante merged commit e1382b7 into refined-github:master Oct 7, 2020
source/features/edit-comments-faster.tsx Outdated Show resolved Hide resolved
@fregante
Copy link
Member

fregante commented Oct 7, 2020

2.3 months in the making

🥳

@yakov116
Copy link
Member

Not sure what broke here. After this PR on #3621 only the first comment has a edit icon. the rest throw a whole bunch of console errors.

@fregante
Copy link
Member

Hidden comments break the feature because they have no reaction button. The observe selector needs to be updated

kidonng added a commit to kidonng/refined-github that referenced this pull request Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

"Edit comment" button injection is not reliable
3 participants