-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
ENH: Add social card previews #88
ENH: Add social card previews #88
Conversation
I believe I've addressed the comments above - here's what I've done:
(I squashed a few commits together to make review easier, thus the force-push) |
368ba64
to
269d082
Compare
I've updated the tests for this, so they should now be passing. The behavior of this PR should only trigger if the user hasn't specified any of the other image options. I've also added a I think this is pretty much ready to go now, as long as a round of reviews is +1 on it. cc @TheTripleV can you please start the test suite? It isn't auto-running for me. |
9528280
to
94dd616
Compare
Anybody know why the
Also, can I just get a note of whether folks are enthusiastic about merging this PR eventually or not? I am perfectly happy to just release this functionality as a standalone Sphinx extension, that was my original intention until I was asked to push this upstream instead. But if folks are busy and don't have time to review or maintain this, that's fine. Just let me know - I know that we are all busy so it's OK if it takes some time, but I don't want to sink a bunch of hours into this if folks aren't enthusiastic about getting the functionality in. |
There's an earlier job called - name: Install dependencies
run: |
set -xe
python -VV
python -m site
python -m pip install --upgrade pip setuptools wheel
python -m pip install -r dev-requirements.txt
- name: Install package
run: |
python -m pip install .
- name: Build wheel
run: |
python -m pip install build
python -m build
- name: Upload sdist and wheel artifacts
uses: actions/upload-artifact@v3
with:
name: my-dist
path: dist/* Then, each of the fresh parallel - name: Install dependencies
run: |
python -VV
python -m site
python -m pip install --upgrade pip setuptools wheel
python -m pip install -r dev-requirements.txt
python -m pip install "sphinx${{ matrix.sphinx-version }}"
- name: Download sdist and wheel artifacts
uses: actions/download-artifact@v3
with:
name: my-dist
path: dist
- name: Install downloaded wheel
run: |
python -m pip install --only-binary=:all: --no-index --find-links=dist sphinxext-opengraph But it's done in a way where it has no access to PyPI, and only installs things already available. So I think you may need to include matplotlib in the "Install dependencies" step of the The I'm also wondering whether always installing matplotlib will cause problems for some, especially if they don't intend to use these generated previews. They'll be able to disable at runtime via |
Sorry for the delays. I started a code review earlier but never finished. I think it looks good. I was going to suggest a few lines of syntax changes but it's mostly lgtm for me. If it's ok with you, I can push those small changes into this branch in about a week or so. |
Replacing the command in "Install downloaded wheel" with someone more like |
Should this also add For example: <meta property="og:image:width" content="1200">
<meta property="og:image:height" content="628"> |
Sure thing, no worries. Feel free to push to this branch. And I understand the delay, I just wanted to make sure folks are interested (and must admit I'd like to use this feature in a number of my sites) Re: installing matplotlib, I can see an argument both ways. On the one hand it is an extra install, on the other this functionality would be best if it happened purely under the hood so that people didn't need to manually turn it on or change their config. Given that matplotlib is such a common library in the python world, maybe we can give a shot at adding the dependency, and then if there are complaints it could be moved to an optional dependency in a future pr? You could also try the same functionality with python image library, but I don't know how to do that which is why I used matplotlib |
We're using this extension for CPython docs:
These images are great, it it would be nice to include them in the CPython docs (at the moment we're just using a single Python logo as the OG image), although there are lots of pages so it generates 482 images, taking up 41 MB (as a separate point, perhaps we can adjust the PNG options to squeeze these a bit). I need to check with others if our deploy is fine with this extra weight. Anyway, we can disable the image build, but one place where Matplotlib being installed by default could be problematic is on our CI. We build Python from
Yes, and Matplotlib has plenty of wheels, but of course doesn't have any yet for Python 3.12, so it might fail building from source (or be very slow). Plus it has dependencies at least for NumPy and Pillow, which are in the same position.
I actually had a go https://github.com/hugovk/pixel-tools/blob/main/og_image.py at that recently before we decided the simple Python logo was better. Your version is much better than what this produces! Anyway, Pillow similarly has lots of wheels but not (yet) 3.12 so is in a similar boat to Matplotlib and NumPy. Having said all this, for our doctest CI, perhaps we could skip the (We're also using this extension for the CPython developer's guide, with 52 images at 4.3 MB. Demo build here: https://hugovk-devguide.readthedocs.io/en/ogp_social_cards/) |
Hey all - just FYI I am going on paternity leave in about 3 or 4 weeks. I'd like to get as many open PRs as possible wrapped up before then. If it doesn't seem like folks have time to look at this before then, I am going to focus my efforts on releasing a separate sphinx extension that uses sphinxext-opengraph under the hood. |
@choldgraf IMO, I'd prefer to have it enabled by default and have an extra dependency requirement. I don't think it's uncommon in the Python world to have cross-dependencies, and the matplotlib API is usually quite stable, so unless a user reports upstream (users should always pin dependencies and sub-dependencies anyways), in-which case we can resolve this on a case-by-case basis. We should resolve the CI issue by adding the matplotlib to the test install steps. Assuming tests pass and a basic functionality check, I'm fine for merge unless @TheTripleV is able to find time to make wanted changes. If that takes too long, it can be a separate PR. I'd prefer not exposing a separate extension to PyPi. My opinion is that the extension should be usable (for us) without configuration. This allows projects like Juypter Books to install this and gain behavior for all users without configuration (assuming a RTD build environment). |
I've added matplotlib to |
@choldgraf can you rebase on upstream, update black, and then run black so that check passes? There also seems to be another test failure |
Once all tests pass, I'll bump to |
I'll fix black in a separate PR. |
Argh the windows builds are off, one second I will fix that. |
@Daltz333 can you try re-running the tests? |
Reran. |
Wanna try one more? I just blackified everything because I realized I'd only run it on the module and not the other python files. |
Reran |
OK I think I figured out why it wasn't fixed before, wanna try one last time? |
@choldgraf Tip: you can also enable GitHub Actions on your own fork at https://github.com/choldgraf/sphinxext-opengraph/actions/ and won't need to wait for someone to press the magic button for them to run there :) |
Oh wow I had no idea that was possible, OK I enabled it over here and will ping you if it's passing so you don't have to keep pressing the button https://github.com/choldgraf/sphinxext-opengraph/actions/runs/4106496890 |
Ok build is happy - try running again! |
Pending testing at wpilibsuite/frc-docs#2189 |
Hm, it looks the extension is not injecting the metadata in both the documentation here and the linked PR above. @choldgraf |
It does look like the metadata is there (look at the value for So for example, for the social cards page here's the value of And if we replace the prefix with the PR preview link to the following: Here it is: |
wooo! |
Looks like it added a new feature: wpilibsuite/sphinxext-opengraph#88
* Add TODOs * Update black * Updated pylint. Disabling new linter "use-dict-literal" for styling reasons. Using a literal would intoduce a new level of indentation. * new pytest * Updating sphinxext-opengraph Looks like it added a new feature: wpilibsuite/sphinxext-opengraph#88
This adds social media card previews to this extension. It's still a work in progress, but opening now for feedback at implementation. If people are +1 then I can improve the docs + tests.
Here's an example of what it looks like.
Here's a demo page:
https://sphinxext-opengraph--88.org.readthedocs.build/en/88/socialcards.html
Implementation
It uses Matplotlib to generate a PNG "social media card" for each page of the documentation and links it via
og:image
(unless an OG image is given by the user manually).It has some basic customization but is meant to work out-of-the-box, and is heavily inspired by GitHub's social card preview design.
To get this working, I also made a few improvements to the development workflow for the theme, which I'll summarize below:
noxfile.py
so that people can build the documentation locally withnox -s docs
(and a live preview withnox -s docs -- live
..ttf
file for Roboto, the Sphinx logo for our defaults.To do
Refs: