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

Pimp lua examples (addresses issues #640 #839) #883

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Mo-Gul
Copy link
Contributor

@Mo-Gul Mo-Gul commented Jun 21, 2020

Closes #640
Closes #755
Closes #881

So far this is done as @hmenke suggested, i.e. the libraries are only shown for the first example if multiple examples are given.
@Mo-Gul Mo-Gul added the manual label Jun 21, 2020
@Mo-Gul Mo-Gul self-assigned this Jun 21, 2020
Copy link
Member

@ilayn ilayn left a comment

Choose a reason for hiding this comment

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

Don't know much about Lua parts but looks good to me (except that CI error and a minor comment).

@@ -243,8 +243,9 @@ \subsubsection{Concept: Chain Groups}

As can be seen, the placement is not particularly nice by default, use the
algorithms from the graph drawing libraries to get a better layout. For
instance, adding |tree layout| to the above code results in the following
somewhat more pleasing rendering:
instance, adding |tree layout| to the above code (and
Copy link
Member

Choose a reason for hiding this comment

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

So this also closes #881 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilayn, sorry, I thought that I had commented on that already, but it seems that didn't work. So here "again".

You are absolutely right. This is, because I did that change in my forks master branch before following your suggestions to do new changes only in branches. A day after doing these changes I thought that I maybe could have avoided that by checking out the commit before I made that change and creating a branch from there.(?)

I hope that I don't have to do everything again, because this is hopefully a trivial change and therefore doesn't mess up anything when this is applied again.

Copy link
Member

Choose a reason for hiding this comment

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

No problem I'll edit this issue text and they will all be closed automagically once this is merged. I think you fought well with git.

@hmenke
Copy link
Member

hmenke commented Jun 23, 2020

The CI failure is a bug in l3backend.

@ilayn
Copy link
Member

ilayn commented Jun 26, 2020

I edited the issue description to close the other relevant issues and PRs. Please go over them once if I did the right ones. Then ready when you are ready.

@Mo-Gul
Copy link
Contributor Author

Mo-Gul commented Jun 26, 2020

The mentioned issues and pull requests are right, thank you. While the last two could already be merged, the first one will still take a while I guess (as is also mentioned in the first comment here).

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

Successfully merging this pull request may close these issues.

Examples in the manual as MWE Manual leaves out \usegdlibrary
3 participants