one-click-review-submission - Restore on new "Files changed" page
#8777
Conversation
|
This is still working on GHE, let me know when this is ready to test and I'll confirm it's still good there. |
one-click-review-submission - Restore featureone-click-review-submission - Restore feature on new Files changed
one-click-review-submission - Restore feature on new Files changedone-click-review-submission - Restore feature on new Files Changed
It works on https://github.com as well if you disable the new files changed experience, which is in preview |
one-click-review-submission - Restore feature on new Files Changedone-click-review-submission - Restore on new "Files changed" page
fregante
left a comment
There was a problem hiding this comment.
Oof, there's a lot going on and a lot that can go wrong on this crucial GitHub feature. I'm not 100% confident that we can/should mess with this
| dialog.style.overflow = 'visible'; | ||
|
|
||
| // actionRow re-renders each time the submit button state changes | ||
| new MutationObserver(syncButtonsDisabledState).observe(actionRow, {childList: true}); |
There was a problem hiding this comment.
This will pile up, a new observer on every click (replaceCheckboxesReact) and is never cleaned up when the feature unloads.
There was a problem hiding this comment.
Can we manually intercept the dialog’s show and hide behavior? Maybe we can intercept its close event and then control its visibility using css.
There was a problem hiding this comment.
This will pile up
I thought about that, but browsers (at least Chromium-based ones) seem smart now - I've inspected the heap snapshot with devtools and didn't find extra observers
There was a problem hiding this comment.
Can we manually intercept the dialog’s show and hide behavior?
I don't think this is possible
Maybe we can intercept its close event
There are no events, it is all React state and conditional returns
When put under this perspective, I think the feature does not deserve to exist, especially with the risk and complexity involved. If it breaks it's an emergency. |
| const dialog = $('div[role="dialog"]'); | ||
| // Desktop layout first, then mobile layout | ||
| const reviewBody = $( | ||
| ['[class^="ReviewMenuButton-module__AnchoredReviewBody"]', '[class^="prc-Dialog-Body"]'], |
There was a problem hiding this comment.
Class order might change without changing functionality, so maybe better use *= instead of ^=? (also in other selectors)
I expected that kind of feedback, and I actually agree with it myself. Especially considering that the new experience is still in preview and keeps changing. I will miss this feature, though
However, I wouldn't say this feature has high potential to break the UI. It will just be difficult to maintain Patching the component may be somewhat more reliable, but this is unlikely to ever happen |
|
Just saying: it still works two months later |
|
You can restore the branch and reopen this PR |
|
I'll keep it as a draft until I thoroughly re-test it |
fixes #8711
this is painful
Test URLs
https://github.com/refined-github/refined-github/pull/8766/files
Screenshot