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

Fix profile-hotkey on Gist pages #3661

Merged
merged 3 commits into from
Oct 17, 2020
Merged

Fix profile-hotkey on Gist pages #3661

merged 3 commits into from
Oct 17, 2020

Conversation

yakov116
Copy link
Member

@yakov116 yakov116 commented Oct 16, 2020

  1. LINKED ISSUES:
    None

  2. TEST URLS:
    https://gist.github.com/yakov116

  3. SCREENSHOT:
    None

It did not work there since /${getUsername() leads to the Your gists button

@@ -5,7 +5,7 @@ import features from '.';
import {getUsername} from '../github-helpers';

function init(): false | void {
const menuItem = select(`a[href="/${getUsername()}"]`);
const menuItem = select.last(`[aria-label="View profile and more"] ~ details-menu a[href$="/${getUsername()}"]`);
Copy link
Contributor

@notlmn notlmn Oct 16, 2020

Choose a reason for hiding this comment

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

Can this be?

Suggested change
const menuItem = select.last(`[aria-label="View profile and more"] ~ details-menu a[href$="/${getUsername()}"]`);
const menuItem = select.last(`[aria-label="View profile and more"] ~ details-menu a[href^="/${getUsername()}"]`);

I mean, both work, but I'm a bit inclined towards ^ for no specific reason.

Copy link
Member Author

@yakov116 yakov116 Oct 16, 2020

Choose a reason for hiding this comment

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

No. The url on the gist page is https://github.com/yakov116
/${getUsername() = https://gist.github.com/yakov116 Which is the link it was catching until not. Hence this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the PR description

This comment was marked as outdated.

Copy link
Member

@fregante fregante Oct 16, 2020

Choose a reason for hiding this comment

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

But in reality this was pretty simple:

Suggested change
const menuItem = select.last(`[aria-label="View profile and more"] ~ details-menu a[href$="/${getUsername()}"]`);
select('a[data-ga-click$="text:your profile"]')!.dataset.hotkey = 'g m';

We don't even need the check. All users have a profile. RG is only expected to work on logged in users.

Copy link
Contributor

Choose a reason for hiding this comment

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

RG is only expected to work on logged in users.

Yes, but at least we can fail gracefully. Would help us as well, if all of a sudden GitHub decides to change class names and other stuff, we can still fail gracefully.

BUT that would beg the question of how we would be able to detect a failure. Errors is how we get most of the reports from users, raising issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

But in reality this was pretty simple:

OMG how did I not catch that?!

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but at least we can fail gracefully.

Nah. Do you really wanna check all the features on logged out pages? I don’t. That’s why we display a warning in that case.

if all of a sudden GitHub decides to change class names and other stuff, we can still fail gracefully.

Unless a feature has the potential to lose data or break native functionality, we don’t need that. At most we display an alert if the feature is a complex action like restore-files

In this case, the previous code silently failed until Yakov went testing all the features; it’s better it let the error appear in the console so someone fixes it.

@yakov116 yakov116 added bug and removed enhancement labels Oct 16, 2020
@yakov116 yakov116 changed the title Enable profile-hotkey on Gist pages Fix profile-hotkey on Gist pages Oct 16, 2020
yakov116 and others added 2 commits October 16, 2020 15:21
Co-authored-by: Federico <me@fregante.com>
@yakov116
Copy link
Member Author

do ga's work on enterprise?

@fregante
Copy link
Member

That’s a good question. We use them in a few features and nobody complained though

@fregante fregante requested a review from busches October 16, 2020 21:33
@fregante
Copy link
Member

Let's merge this in the meanwhile. We can decide later what we can do about GHE support (if anything)

@fregante fregante merged commit f395e1f into master Oct 17, 2020
@fregante fregante deleted the profile_hotkey_gist branch October 17, 2020 02:19
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.

None yet

3 participants