Improve reliability of clean-conversation-headers - #5322
Conversation
fregante
left a comment
There was a problem hiding this comment.
Good stuff if the result is equivalent! 👌
Not exactly the same since we are comparing literal whitespaces with margins etc. But the differences should be pretty minimal. |
clean-conversation-headersclean-conversation-headers more reliable
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
clean-conversation-headers more reliableclean-conversation-headers
| position.after( | ||
| <details | ||
| className={`details-reset details-overlay d-inline-block position-relative ${dropdownClass}`} | ||
| className={`details-reset details-overlay d-inline-block ml-2 position-relative ${dropdownClass}`} |
There was a problem hiding this comment.
Fixed the spacing of the elements added by RGH.
Before:
After:
fregante
left a comment
There was a problem hiding this comment.
We should keep awaitDomReady: false
| async function init(): Promise<void | Deinit> { | ||
| const cleanConversationHeader = pageDetect.isIssue() ? cleanIssueHeader : cleanPrHeader; | ||
|
|
||
| // Wait for the initial clean-up to finish before setting up the observer #5573 | ||
| if ((await cleanConversationHeader()) !== false) { | ||
| return onConversationHeaderUpdate(cleanConversationHeader); | ||
| } | ||
| } |
There was a problem hiding this comment.
Seems like the onConversationHeaderUpdate observer was set up too soon and ended up being triggered by the feature itself.
This makes the feature much more reliable and doesn't sacrifice speed 🏄 🌊
| .gh-header-meta .rgh-conversation-activity-filter-wrapper { | ||
| margin-bottom: 8px; /* Preserve centered vertical alignment with status badge */ | ||
| } |
There was a problem hiding this comment.
Before
After
(It's easier to add via CSS than via Primer because we only want this in the "main" header, not the sticky one.)
|
Thanks for finishing this @cheap-glitch! However the long merge window seems to caused some confusion and reverted parts of #5416 (specifically regarding |
|
I think you're specifically referring to the blue background of
It took me a while to understand that 😂 |
|
Yup, I was supposed to submit a PR right after commenting 😅 |
Fixes #5573
Goals for this PR:
clean-conversation-headersdisplays incorrectly #5321, which is caused byincludeandadditionalListenersboth triggering.Test URLs
clean-conversation-headers#4152 except for "Open PR (default branch)", which can be replaced by this PR 😉clean-conversation-headersdisplays incorrectly #5321: https://github.com/peopledoc/septentrion/pull/136/files/346da19f7cd57956281751f33665c52d128de588Screenshot
Before:
After: