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 dimensions of mastodon icon #487

Merged
merged 7 commits into from
Mar 11, 2023
Merged

Conversation

miltondp
Copy link
Contributor

@miltondp miltondp commented Mar 4, 2023

This PR changes the dimensions of the Mastodon icon so when the manuscript is converted to docx the image has the right size. @vincerubinetti might want to check this just in case.

This is how it looks right now and how it looks with this change:

image

image

@miltondp miltondp marked this pull request as ready for review March 4, 2023 01:49
@AppVeyorBot
Copy link

AppVeyor build 1.0.318 for commit 5c38bb8 is now complete.

Found 51 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscing
content/02.delete-me.md:44:aliqua
content/02.delete-me.md:44:amet
content/02.delete-me.md:44:consectetur
content/02.delete-me.md:44:dolore
content/02.delete-me.md:44:eiusmod
content/02.delete-me.md:44:elit
content/02.delete-me.md:44:incididunt
content/02.delete-me.md:44:ipsum
content/02.delete-me.md:44:labore
content/02.delete-me.md:44:Lorem
content/02.delete-me.md:44:magna
content/02...
The rendered manuscript from this build is temporarily available for download at:

@AppVeyorBot
Copy link

AppVeyor build 1.0.319 for commit 13dae3c is now complete.

Found 51 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscing
content/02.delete-me.md:44:aliqua
content/02.delete-me.md:44:amet
content/02.delete-me.md:44:consectetur
content/02.delete-me.md:44:dolore
content/02.delete-me.md:44:eiusmod
content/02.delete-me.md:44:elit
content/02.delete-me.md:44:incididunt
content/02.delete-me.md:44:ipsum
content/02.delete-me.md:44:labore
content/02.delete-me.md:44:Lorem
content/02.delete-me.md:44:magna
content/02...
The rendered manuscript from this build is temporarily available for download at:

@vincerubinetti
Copy link
Collaborator

Let's take this opportunity to make the icon consistent with the others. Here is cleaner code from Font Awesome:

<!-- Modified from https://fontawesome.com/icons/mastodon under its CC BY 4.0 License -->
<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 448 512">
	<path fill="#808080" d="M433 179.11c0-97.2-63.71-125.7-63.71-125.7-62.52-28.7-228.56-28.4-290.48 0 0 0-63.72 28.5-63.72 125.7 0 115.7-6.6 259.4 105.63 289.1 40.51 10.7 75.32 13 103.33 11.4 50.81-2.8 79.32-18.1 79.32-18.1l-1.7-36.9s-36.31 11.4-77.12 10.1c-40.41-1.4-83-4.4-89.63-54a102.54 102.54 0 0 1-.9-13.9c85.63 20.9 158.65 9.1 178.75 6.7 56.12-6.7 105-41.3 111.23-72.9 9.8-49.8 9-121.5 9-121.5zm-75.12 125.2h-46.63v-114.2c0-49.7-64-51.6-64 6.9v62.5h-46.33V197c0-58.5-64-56.6-64-6.9v114.2H90.19c0-122.1-5.2-147.9 18.41-175 25.9-28.9 79.82-30.8 103.83 6.1l11.6 19.5 11.6-19.5c24.11-37.1 78.12-34.8 103.83-6.1 23.71 27.3 18.4 53 18.4 175z" />
</svg>

Also preserving aspect ratio doesn't really matter in this case, as the svg will scale to be contained within the shorter dimension without stretching. That is, keeping it 16x16 will just add a tiny bit of white space on either side, in this case.

In the future, or maybe we can do it in this PR, it'd be better to only specify the height, and to specify it in terms of the text size it is within, i.e. height="1em".

@agitter
Copy link
Member

agitter commented Mar 4, 2023

Thanks for working on this @miltondp.

it'd be better to only specify the height, and to specify it in terms of the text size it is within

Can we address that in this same pull request? @vincerubinetti is the proposal to edit the height in the svg files and leave the relevant parts of content/00.front-matter.md as

![ORCID icon](images/orcid.svg){.inline_icon width=16 height=16}

for all icons?

@vincerubinetti
Copy link
Collaborator

vincerubinetti commented Mar 4, 2023

Apologies, I remember what is going on now.

We already have an .inline_icon class that sets the height to 1em, but because that CSS (in themes/default.html) is loaded at the end of the <body>, it won't get applied until a few hundred milliseconds after the content of <body> has already been shown. This leads to a momentary flash where the icons are huge because they have no set dimensions. So I probably had us add width="16" height="16" to the icons themselves as a fallback, just for that momentary flash. 1em (or any other font size) will override it once the style loads, and preserve aspect ratio.

tl;dr, just add width="16" height="16" to each svg icon, and only attach {.inline_icon} when using it. No more is needed.

Also for consistency, check Font Awesome first for any icons you add in the future (I know, FA probably didn't have Mastodon right away). You'll get a nice clean single <path> element and matching viewBox, and that's all you need.

@AppVeyorBot
Copy link

AppVeyor build 1.0.322 for commit 4ff1483 is now complete.

Found 51 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscing
content/02.delete-me.md:44:aliqua
content/02.delete-me.md:44:amet
content/02.delete-me.md:44:consectetur
content/02.delete-me.md:44:dolore
content/02.delete-me.md:44:eiusmod
content/02.delete-me.md:44:elit
content/02.delete-me.md:44:incididunt
content/02.delete-me.md:44:ipsum
content/02.delete-me.md:44:labore
content/02.delete-me.md:44:Lorem
content/02.delete-me.md:44:magna
content/02...
The rendered manuscript from this build is temporarily available for download at:

@miltondp
Copy link
Contributor Author

miltondp commented Mar 6, 2023

I hope I got @vincerubinetti right and made the correct changes.

@AppVeyorBot
Copy link

AppVeyor build 1.0.323 for commit 837c57e is now complete.

Found 51 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscing
content/02.delete-me.md:44:aliqua
content/02.delete-me.md:44:amet
content/02.delete-me.md:44:consectetur
content/02.delete-me.md:44:dolore
content/02.delete-me.md:44:eiusmod
content/02.delete-me.md:44:elit
content/02.delete-me.md:44:incididunt
content/02.delete-me.md:44:ipsum
content/02.delete-me.md:44:labore
content/02.delete-me.md:44:Lorem
content/02.delete-me.md:44:magna
content/02...
The rendered manuscript from this build is temporarily available for download at:

@vincerubinetti
Copy link
Collaborator

The width and height next to .inline_icon is not necessary.

@@ -52,7 +52,7 @@ Published: {{manubot.date_long}}
{%- endif %}
{%- if author.mastodon is defined and author.mastodon is not none and author["mastodon-server"] is defined and author["mastodon-server"] is not none %}
{%- set has_ids = true %}
· ![Mastodon icon](images/mastodon.svg){.inline_icon}
· ![Mastodon icon](images/mastodon.svg){.inline_icon width=16 height=16}
Copy link
Member

Choose a reason for hiding this comment

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

@vincerubinetti moving your comment here:

The width and height next to .inline_icon is not necessary.

I see that we have width and height set for the other icons here as well. So assuming we should delete them for all? I wonder if there was some utility of them, like for the docx export? I'll dig through our past conversations.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we added the width=16 height=16 from prior to when we had implemented this solution:

tl;dr, just add width="16" height="16" to each svg icon, and only attach {.inline_icon} when using it. No more is needed.

Copy link
Collaborator

@vincerubinetti vincerubinetti Mar 7, 2023

Choose a reason for hiding this comment

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

like for the docx export?

That very well may be. Sorry, I always forget about docx. If you could do a quick local test (my machine was recently wiped and I can't quickly run Manubot atm)... remove the dimensions next to .inline_icon and see if the docx displays properly in Word. If Word Pandoc is smart enough, it could set the dimensions from the inherent SVG dimensions (in the SVG source) if dimensions on the <img> element are missing.

Regardless though, the html/pdf exports should only need one or the other. If docx needs dimensions next to .inline_icon, we can get rid of them in the SVGs themselves.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it looks like these were added in #439 to fix the docx problems reported in #437.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I confirm that if I remove the dimensions next to .inline_icon, then all icons look big and blurry in the docx file (see below), but the HTML/PDF exports work as expected.

image

If I leave the dimensions next to .inline_icon but remove them from the SVG, then everything works in all exports and the icons look even better in docx (see below, before/after):

output

Copy link
Collaborator

@vincerubinetti vincerubinetti Mar 10, 2023

Choose a reason for hiding this comment

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

Thanks for checking and doing this PR Milton. Can you enable editing of your fork so I can make some final tweaks?

Nevermind I didn't realize I'm not a maintainer and can't make changes anyway. Instead, could you just drop these into the repo:

icons.zip

Font Awesome now has ORCID (with its path cleaned up and shorter). And also made the path tags all self-closing to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vincerubinetti thank you, just pushed those changes. We should be ready to merge.

miltondp added a commit to greenelab/phenoplier_manuscript that referenced this pull request Mar 7, 2023
@AppVeyorBot
Copy link

AppVeyor build 1.0.325 for commit 0ffdba6 is now complete.

Found 51 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscing
content/02.delete-me.md:44:aliqua
content/02.delete-me.md:44:amet
content/02.delete-me.md:44:consectetur
content/02.delete-me.md:44:dolore
content/02.delete-me.md:44:eiusmod
content/02.delete-me.md:44:elit
content/02.delete-me.md:44:incididunt
content/02.delete-me.md:44:ipsum
content/02.delete-me.md:44:labore
content/02.delete-me.md:44:Lorem
content/02.delete-me.md:44:magna
content/02...
The rendered manuscript from this build is temporarily available for download at:

Co-authored-by: Vincent Rubinetti <vince.rubinetti@gmail.com>
@AppVeyorBot
Copy link

AppVeyor build 1.0.326 for commit 4ba9cd3 is now complete.

Found 51 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscing
content/02.delete-me.md:44:aliqua
content/02.delete-me.md:44:amet
content/02.delete-me.md:44:consectetur
content/02.delete-me.md:44:dolore
content/02.delete-me.md:44:eiusmod
content/02.delete-me.md:44:elit
content/02.delete-me.md:44:incididunt
content/02.delete-me.md:44:ipsum
content/02.delete-me.md:44:labore
content/02.delete-me.md:44:Lorem
content/02.delete-me.md:44:magna
content/02...
The rendered manuscript from this build is temporarily available for download at:

Copy link
Member

@agitter agitter left a comment

Choose a reason for hiding this comment

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

Will let @dhimmel take another look and then merge.

@dhimmel dhimmel merged commit 26bec7c into manubot:main Mar 11, 2023
danich1 pushed a commit to danich1/word_lapse_manuscript that referenced this pull request Apr 5, 2023
merges manubot/rootstock#487

* add width/height to mastodon icon in front-matter
* update mastodon icon from Front Awesome
* remove dimensions from SVG files, since these are specified in
   the front matter.

Co-authored-by: Vincent Rubinetti <vince.rubinetti@gmail.com>
@miltondp miltondp deleted the mastodoc-icon-fix branch June 1, 2023 19:13
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.

5 participants