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

Add orcidlinks, orcidlinkm, orcidlinkl #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jonas-schulze
Copy link

  • small: current/old size
  • medium: matches looks of stackoverflow post
  • large: should match large variant from ORCID guidelines (preview.png)

@jonas-schulze
Copy link
Author

Where should I document these new variants?

Shall I refactor the variable names in favor of e.g. \ORCID@TargetHeight, \ORCID@TargetDepth, etc.?

I just noted that I created the commit on my master. If you prefer a PR from a different/feature branch, I'm happy to open a new PR (unless it is possible to change the base branch of a PR).

* small: current/old size
* medium: matches looks of stackoverflow post
* large: should match large variant from ORCID guidelines (preview.png)
@duetosymmetry
Copy link
Owner

Hi @jonas-schulze, this looks like it could be useful! But for backwards compatibility, the previous command \orcidlink{ORCID} should not change the number of arguments that it takes.

@jonas-schulze
Copy link
Author

jonas-schulze commented Dec 14, 2022

I haven't used \DeclareRobustCommand before, but doesn't \orcidlink take the same number of arguments as \orcidlinks, which is one, as before?

@duetosymmetry
Copy link
Owner

Sorry that I wasn't paying close enough attention before, I missed that you created a new internal command \@orcidlink, not redefined the external command. I'll have to take a closer look and see what's going on (though I'm on vacation right now!).

Can you comment on why both \fontcharht and \fontchardp are needed? Is this something that I should have been doing before, which is distinct from allowing multiple sizes?

Also, do you have any thoughts on how to minimize the code duplication between \@orcidlink and \orcidlinkl?

@jonas-schulze
Copy link
Author

jonas-schulze commented Dec 15, 2022

No rush, we can talk about this after your vacation, or next year even. (A brief heads-up would be nice, though.) 🙂

Can you comment on why both \fontcharht and \fontchardp are needed? Is this something that I should have been doing before, which is distinct from allowing multiple sizes?

\fontcharht only measures the "size" above the baseline, \fontcharht only the one below.
So, the total "size" of a glyph is the sum of its height and depth.
The t-shirt sizes S and M scale the logo to match size and positioning of a capital X and a bar |, respectively.
The X has zero depth, so \fontchardp wasn't needed before.
However, it does no harm either when reusing the logic for size M.

Also, do you have any thoughts on how to minimize the code duplication between @orcidlink and \orcidlinkl?

I have already thought about that, and came to no ideal solution.
The size L scales the logo such that the size (=height) of the text within the logo matches the size and positioning of a lowercase x.
The internal \@orcidlink could be refactored to take the scaling factor as well,
\@OrigHeightRecip for S/M or \@OrigInnerHeightRecip for L,
but I haven't been able to achieve the desired baseline in a technique similar to S/M.
Therefore, \@orcidlink would have to take an argument for the TikZ baseline as well,
while S/M have a superfluous \coordinate defined.
Overall, I would consider this to be over-engineered for this simple of a task.

`curX*` becomes `Target*` as it isn't related to a capital X anymore.
`*height` becomes `*Size` as it contains height and depth of a glyph.
@jonas-schulze
Copy link
Author

Hi @duetosymmetry, happy new year! I hope you've had a nice vacation. I've just returned from vacation myself. ⛷️
Have you had time to have a closer look at this PR?

@jonas-schulze
Copy link
Author

bump 🙂

1 similar comment
@jonas-schulze
Copy link
Author

bump 🙂

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