-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix deprioritize-marketplace-link
#3725
Conversation
@busches can you double check this on enterprise? |
Co-authored-by: Federico <me@fregante.com>
Oh my... that applied bad. 🤣 |
?.closest('.border-top, .mr-3')!.remove(); | ||
|
||
(await elementReady('[aria-label="View profile and more"]')) | ||
?.closest('details')! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, is the ?.
needed in either one? If Enterprise doesn't have the link, it should just be excluded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure.
I think it was added because some pages, like the enter password dont have the links. I dont know if its still relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like the enter password dont have the links
Let's find out because I don't care about password pages. They have nothing so, if anything, RGH should not run there at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the code should be updated to use a regular if:
If there’s a marketplace link, move it to the dropdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a comment
@yakov116 This is not always a thing on Enterprise, I believe it's opt-in only at this point, keeping the |
|
||
await domLoaded; | ||
|
||
function handleMenuOpening(): void { | ||
select.last('.header-nav-current-user ~ .dropdown-divider')!.before( | ||
<div className="dropdown-divider"/>, | ||
<a className="dropdown-item" href="/marketplace">Marketplace</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@busches Does GHE link here or always to github.com? If github.com, let's use this:
<a className="dropdown-item" href="/marketplace">Marketplace</a> | |
<a className="dropdown-item" href="https://github.com/marketplace">Marketplace</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I've not seen it activated in the wild, just sounds possible from the release notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who knows then. The selector only specifically selects local links (.Header-link[href="/marketplace"]
) so if this change is required, the selector also needs to be changed.
Since nobody complained, I'll leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the current implementation works for you, it can be merged
LINKED ISSUES:
profile-hotkey
is broken since the dropdown is lazy loaded #3720 (comment)TEST URLS:
all over
SCREENSHOT:
None