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

Use Font Awesome to provide a cute icon #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

embray
Copy link
Contributor

@embray embray commented Feb 27, 2018

Another little patch from my instance of the plugin. Adds the GitHub icon next to the "GitHub Login" link to better distinguish it, and make clearer that it will send users to GH.

@embray
Copy link
Contributor Author

embray commented Feb 27, 2018

You can see an example of how this looks at https://trac.sagemath.org/

@rjollos rjollos requested review from rjollos and removed request for rjollos February 27, 2018 21:33
@@ -28,7 +28,7 @@
from trac.versioncontrol.web_ui.changeset import ChangesetModule
from trac.web.api import IRequestHandler, RequestDone
from trac.web.auth import LoginModule
from trac.web.chrome import add_warning
from trac.web.chrome import add_warning, add_script
Copy link
Member

Choose a reason for hiding this comment

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

Order of these should be switched per PEP8. No need to recreate pull request, I'll make the change before merging.

doc='This plugin uses Font Awesome to provide the GitHub icon. '
'Set the URL to the FA JavaScript to use here, or set to empty '
'to disable the icon altogether.')

Copy link
Member

Choose a reason for hiding this comment

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

I would change comment to:

Add a GitHub icon to the login/logout button. The icon is enabled by specifying the path to the Font Awesome script. Set the value to empty to remove the icon.

@rjollos
Copy link
Member

rjollos commented Feb 27, 2018

Something to consider is whether we want to include the Font Awesome script in the plugin (see ITemplateProvider) so that it can be served directly. If we choose to not do that, it could still be served directly by adding the script to the Environment or shared htdocs directory and using a path starting with /chrome/site/.

@aaugustin
Copy link
Contributor

I'd rather not add several hundred kilobytes of Font Awesome to every page for loading a single icon.

Can we load only the icon we need, one way or another?

@embray
Copy link
Contributor Author

embray commented Feb 28, 2018

@rjollos @aaugustin I've thought about both these things. I definitely agree FA is overkill, it was just the quickest and easiest way to do this. What I do like about it is that it's an actual font glyph and scales with the text nicely, but I've heard of ways to extract just the bits you need out of FA. I could look into that.

For my purposes it was fine, but I agree with these points.

@rjollos
Copy link
Member

rjollos commented Mar 3, 2018

Good point by @aaugustin. Assuming we can use a smaller script, I think it would be reasonable to bundle the script in the plugin and remove the option. If someone really dislikes the icon they can customize their interface to remove it.

@aaugustin
Copy link
Contributor

http://fontello.com/ make is super easy to build a custom subset of Font Awesome — in that case, a subset with only the icon you need.

Copy link
Member

@neverpanic neverpanic left a comment

Choose a reason for hiding this comment

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

Like the change in general, but I would really prefer if we didn't load external resources for that and locally copied the icon.

From a MacPorts point of view, I don't want to expose our users to tracking or the additional downloads from third party servers.

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.

None yet

5 participants