Skip to content

manimce to manim & capitalizing Manim in readme #794

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

Merged
merged 10 commits into from
Dec 4, 2020

Conversation

kolibril13
Copy link
Member

@kolibril13 kolibril13 commented Nov 30, 2020

I think as we have now a new Pypi location, we should update the readme and the docs.
Further, at the moment there was some inconsistency in Capitalization in the readme that is now fixed as well.

One further thing I might add to this PR is another highlighting colour in the docs for everything that goes into " ` " , as I don't like the orange and red words together in one part: EDIT: For a later pr
xxx

@kolibril13 kolibril13 added documentation Improvements or additions to documentation pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! labels Nov 30, 2020
Copy link
Member

@naveen521kk naveen521kk left a comment

Choose a reason for hiding this comment

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

Why don't you also edit the pyproject.toml and also update the lock file using poetry lock command after editing? Or else on release things will be uploaded only to manimce and not manim.

@@ -12,7 +12,7 @@ You can install manim very easily using chocolatey, by typing the following comm

.. code-block:: powershell

choco install manimce
choco install manim
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this line. It's manimce only, because of some version issue manim is deprecated on chocolatey, so it is correct to say manimce here.

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, I will change it back to manimce. will this change in future?

Copy link
Member

Choose a reason for hiding this comment

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

Only after we do a 1.0.0 release it will change.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@behackl
Copy link
Member

behackl commented Nov 30, 2020

Regarding the color change of code literals: I don't mind the current color scheme; I'm open to experiments.

Just some general comments off the top of my head:

  • I feel it is important that code literals are clearly distinguished from text; I worry that simply changing color to something "normal" like gray or black will bring code literals too close to normal text (but you'd have to try how it looks; maybe a GitHub-esque solution with additionally colored background could be a solution).
  • The reason I actually like that these are currently orange/red/whatever is that it is close to our link color -- which is good, because many of the code literals are links at the same time. If there is some color involved, I think that it should be close to our link color.
  • It could even make sense to visually distinguish between code literals that are links, and those that are not. In that case, I could very well live with "non-linking" code literals that look similar to the ones here on GitHub.

@kolibril13
Copy link
Member Author

  • The reason I actually like that these are currently orange/red/whatever is that it is close to our link color -- which is good, because many of the code literals are links at the same time. If there is some color involved, I think that it should be close to our link color.

We could also make both orange and the links could be underlined.

But I'd prefer to distinguish the only highlighted things form the linked things in a way, and I think that bold would also look quite nice and easy to distinguish and I also like the idea of making it similar to the ones here on GitHub.

@kolibril13
Copy link
Member Author

Why don't you also edit the pyproject.toml and also update the lock file using poetry lock command after editing? Or else on release things will be uploaded only to manimce and not manim.

I've now also made the change in pyproject.toml , and there are now lots of changes in poetry.lock.
Can maybe someone with more experience have a look at that?

Copy link
Member

@naveen521kk naveen521kk left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

This looks good to me so far. There are further occurrences in the current master branch that are not modified here:

  • version_num = pkg_resources.get_distribution("manimce").version in the twitter template (master needs to be merged in here for this to work)
  • (either using ``pip install manimce`` or with Chocolatey) in troubleshooting.rst
  • asset_name: manimce-${{ steps.tag.outputs.tag }}.tar.gz in python-publish.yml

The first two occurrences should definitely be changed; I'm not sure about the third one.

@naveen521kk naveen521kk mentioned this pull request Dec 2, 2020
@kolibril13
Copy link
Member Author

  • asset_name: manimce-${{ steps.tag.outputs.tag }}.tar.gz in python-publish.yml

ok, I also updated this now, however, I have no idea of what this does. @eulertour : can you tell us if this should be changed?

Copy link
Member

@naveen521kk naveen521kk left a comment

Choose a reason for hiding this comment

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

I think this is done.

Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

Left a few non-blocking comments; maybe leave the PR open for a little while longer before merging, in case anyone else has something to add about it.

@@ -47,5 +47,5 @@ jobs:
with:
upload_url: ${{ steps.create_release.outputs.upload_url }} # This pulls from the CREATE RELEASE step above, referencing it's ID to get its outputs object, which include a `upload_url`. See this blog post for more info: https://jasonet.co/posts/new-features-of-github-actions/#passing-data-to-future-steps
asset_path: dist/*.tar.gz
asset_name: manimce-${{ steps.tag.outputs.tag }}.tar.gz
asset_name: manim-${{ steps.tag.outputs.tag }}.tar.gz
Copy link
Member

Choose a reason for hiding this comment

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

I think that this will require an update from @Groctel for the AUR pkgbuild at https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=manimce#n36 -- but if I read the pkgbuild correctly, it will have to be updated for the new release anyways, so not actually a problem.

<a href="http://choosealicense.com/licenses/mit/"><img src="https://img.shields.io/badge/license-MIT-red.svg?style=flat" alt="MIT License"></a>
<a href="https://www.reddit.com/r/manim/"><img src="https://img.shields.io/reddit/subreddit-subscribers/manim.svg?color=orange&label=reddit" alt="Reddit" href=></a>
<a href="https://github.com/psf/black"><img src="https://img.shields.io/badge/code%20style-black-000000.svg" alt="Code style: black">
Copy link
Member

Choose a reason for hiding this comment

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

Comment, non-blocking: I'm not sure whether we actually need a black badge in our readme, but won't block the PR because of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

same idea here, that was inspired by other repos:
https://github.com/pandas-dev/pandas

<a href="https://discord.gg/mMRrZQW"><img src="https://img.shields.io/discord/581738731934056449.svg?label=discord&color=yellow" alt="Discord"></a>
<a href="https://manimce.readthedocs.io/en/latest/"><img src="https://readthedocs.org/projects/manimce/badge/?version=latest" alt="Documentation Status"></a>
<a href="https://hub.docker.com/r/manimcommunity/manim"><img src="https://img.shields.io/docker/v/manimcommunity/manim?color=%23099cec&label=docker%20image" alt="Docker image"> </a>
<a href="https://pepy.tech/project/manim"><img src="https://pepy.tech/badge/manim/month?" alt="Downloads"> </a>
Copy link
Member

Choose a reason for hiding this comment

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

Comment, non-blocking: Same with this, download stats aren't something I as a (potential) user would be interested in.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it gives nice feedback about how often this repo is used, and I was inspired by other repos that have this badge as well:
https://github.com/matplotlib/matplotlib
https://github.com/pandas-dev/pandas

@kolibril13
Copy link
Member Author

Left a few non-blocking comments; maybe leave the PR open for a little while longer before merging, in case anyone else has something to add about it.

I'd rather merge it now so that manim on pypi can be updated.
But we can definitely make followup prs on this.

@kolibril13 kolibril13 merged commit 7fe79e3 into ManimCommunity:master Dec 4, 2020
@kolibril13 kolibril13 deleted the docs_new_color branch December 4, 2020 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants