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

Extension: Change button icon, fix popup and double quote #64

Merged
merged 2 commits into from
Aug 3, 2023
Merged

Extension: Change button icon, fix popup and double quote #64

merged 2 commits into from
Aug 3, 2023

Conversation

WarningImHack3r
Copy link
Contributor

Bring UI tweaks to the Chrome extension's in-GitHub button:

  • Change the search SVG to GitHub's one (from their new UI, in the top search bar)
  • Fix the tooltip not working (anymore?) by adding popover="manual" in the button's HTML

Also, change the remaining simple quotes to double quotes for consistency

@payne911
Copy link
Collaborator

payne911 commented Jul 31, 2023

Thanks for your contribution!

Would you mind including a little screenshot in your PR's summary? :)

Also, change the remaining simple quotes to double quotes for consistency

Ideally, I would have preferred that being done as a separate commit, but it's not really a big deal.

@WarningImHack3r
Copy link
Contributor Author

WarningImHack3r commented Jul 31, 2023

Thanks for your contribution!

No problem :)

Would you mind including a little screenshot in your PR's summary? :)

Here you are!
CleanShot 2023-07-31 at 14 17 42@2x

Note
I know it's not perfectly vertically centered, but none of the original GitHub buttons really are, so I guess it's a styling issue on GitHub's side they probably would fix by themselves

Also, change the remaining simple quotes to double quotes for consistency

Ideally, I would have preferred that being done as a separate commit, but it's not really a big deal.

Sorry!

@payne911
Copy link
Collaborator

payne911 commented Aug 2, 2023

Thanks!
I hear you on the "GitHub might fix this eventually", but I think it'd be nice to just have it centered right now, and if they ever do fix this we adjust accordingly.
Would you mind trying to center it vertically a bit more?

@WarningImHack3r
Copy link
Contributor Author

WarningImHack3r commented Aug 2, 2023

The thing is it would very likely require (inline) CSS and that would be the first "hardcoded" snippet on that injected HTML, which I'm not a fan of.
I can possibly try to find an already available (= compiled) Tailwind class somewhere (what all this button is already styled with) to make it "cleaner", but even there we can't be ensured it will stay available forever...
A second reason not to do so is that the current magnifying glass icon is not centered either, so it's not a regression, just a better look but still uncentered.

TL;DR: I can do it with inlined CSS if you want but that's not an idea I like much

@payne911
Copy link
Collaborator

payne911 commented Aug 3, 2023

Wouldn't another approach be to simply change the values of the SVG?

To me, this icon looks properly centered:
image

Whereas this one somehow seems a bit off-centered:
image

Honestly, it's no big deal. But if there's a quick hack for it, I think it'd be nice. Let me know if you'd rather me simply merge this as-is.

@payne911
Copy link
Collaborator

payne911 commented Aug 3, 2023

Alright, tested it locally and it looks good enough. Let's not bother you further with this.

image

Thanks again for your contribution! 👍
And while I'm here: if you ever feel like it and have some free time, know that #59 is definitely the highest priority issue right now.

@payne911 payne911 merged commit c026c2c into useful-forks:master Aug 3, 2023
@WarningImHack3r
Copy link
Contributor Author

Thank you! Oh I didn't look at issues at all, gonna take a quick look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants