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 multiline formatting bug #11

Merged
merged 4 commits into from
Feb 15, 2023
Merged

Conversation

lenianiva
Copy link
Contributor

@lenianiva lenianiva commented Jan 20, 2023

Resolves #2

@lenianiva
Copy link
Contributor Author

This branch replaces \eqnannotationtext with \eqnannotationfont and \eqnannotationstrut with the font part provided to \node[font=\eqnannotationfont]. I have not completely modified the example and PDF yet in order to showcase that it works as intended.

@st--
Copy link
Owner

st-- commented Jan 20, 2023

Great, thank you so much for contributing - I'll see how it works :)

@st--
Copy link
Owner

st-- commented Feb 8, 2023

Just to let you know I'm still intending to sort this out!

@lenianiva
Copy link
Contributor Author

lenianiva commented Feb 8, 2023

Thanks! (sorry i replied to the wrong thread that is why you see the edit)

Copy link
Owner

@st-- st-- left a comment

Choose a reason for hiding this comment

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

Hi @vleni ! Thank you so much for fixing this issue. This is basically good to go, I just marked a few changes, mainly for consistency - most of it as review suggestions, so you could just go and accept them from GitHub. The only other thing I couldn't "suggest" directly was removing the now-obsolete workaround - I've left a comment about that.

If you're happy with my suggestions, ping me and I'll merge it in. :)

annotate-equations.sty Outdated Show resolved Hide resolved
annotate-equations.tex Outdated Show resolved Hide resolved
annotate-equations.tex Outdated Show resolved Hide resolved
annotate-equations.tex Outdated Show resolved Hide resolved
annotate-equations.tex Outdated Show resolved Hide resolved
annotate-equations.tex Outdated Show resolved Hide resolved
annotate-equations.tex Outdated Show resolved Hide resolved
@@ -311,7 +335,6 @@ \section{Known issues}
\begin{itemize}
\item \texttt{label above}/\texttt{label below} is not implemented for \verb|\annotate|.

\item Formatting only covers first line in multi-line annotation texts (see \cref{sec:multiline}).
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for fixing it 🎉

@@ -263,7 +287,7 @@ \subsection{Line breaks within annotations}
\end{LTXexample}
\noindent
%
Here is a manual work-around:
Here is a manual work-around: (obsolete)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it's helpful to show the old work-around - could you just delete it instead? All it needs to say here is that using \\ within the text to get a line break works just fine.:)

@lenianiva
Copy link
Contributor Author

Thanks! It should be ok now after your change.

@st--
Copy link
Owner

st-- commented Feb 14, 2023

@vleni if you agree with all the suggestions, you can just go and accept them (see GitHub docs in case that's new to you).

Also, would you be happy to adapt the no-longer-needed section on the workaround (see https://github.com/st--/annotate-equations/pull/11/files#r1105454093) instead of just marking it "obsolete"?

If you prefer, I can also do all those changes myself, but I didn't want to do so without giving you the chance first because it would seem rude to me to edit someone else's contributions for them:)

@st--
Copy link
Owner

st-- commented Feb 14, 2023

(Side note- I edited the PR description to include "resolves #2" because a) the link to the issue makes it easier to keep up with why this PR is here and what it does and b) GitHub has this neat feature where merging a PR also closes the linked issues :))

@st--
Copy link
Owner

st-- commented Feb 14, 2023

Oh, one more task: after the final changes, could you also regenerate the PDF & commit that?

Update: I spotted a minor typography issue in the TeX that I fixed on the main branch - you can either merge that into your branch and then regenerate the PDF, or skip generating the PDF & I'll do it separately after merging your PR - either works:)

@lenianiva
Copy link
Contributor Author

ok I fixed all of the suggestions and I regenerated the PDF but I didn't merge master. Could you do it in the merge commit?

Copy link
Owner

@st-- st-- left a comment

Choose a reason for hiding this comment

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

looks good!

annotate-equations.tex Outdated Show resolved Hide resolved
annotate-equations.tex Outdated Show resolved Hide resolved
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.

formatting of multi-line annotation text only applies to first line (answer: now working)
2 participants