Add banner feature to qiskit theme

Qiskit / qiskit_sphinx_theme

A Sphinx theme and documentation guidelines for Qiskit and Qiskit Ecosystem projects

https://qisk.it/docs-guide

Apache License 2.0

14 stars 29 forks source link

Add banner feature to qiskit theme #530

Closed javabster closed 1 year ago

javabster commented 1 year ago

In this PR:

[x] add optional purple gradient banner to all pages
[x] add optional "Learn more" link to banner
[x] update readme
[x] works for mobile view
[ ] snapshot tests for banner component

I'm struggling to figure out how to add new snapshots for this component, any help appreciated!

MaldoAlberto commented 1 year ago

@javabster is only add the images in the snapshot test?

javabster commented 1 year ago

@MaldoAlberto the issue I'm facing is that new screenshots are not being generated for the announcement banner tests 😅 I'm not sure what's going wrong, possibly because the docker setup is not realising that the option has been set in the conf.py? I'm not really sure

MaldoAlberto commented 1 year ago

@javabster I saw the images from announcement is not created, because not exist a html file to obtain the snapshot, the others I found in example_docs/docs/_build/html

javabster commented 1 year ago

yeah I don't really understand what is happening with these snapshots tbh, when I was doing my dev work locally I was able to build with the announcement banner in the example_docs, but for some reason when I try run the snapshots workflow (I'm trying to follow the instructions in the contributing guide) the banner isn't getting generated for some reason? I'm trying to figure out how the docker image is getting built but I'm a bit lost tbh.

Also no idea why the CI snapshots keep failing when the "actual" images look exactly the same as the originals 😅

MaldoAlberto commented 1 year ago

I found the path for the announcement is example_docs/docs/_ecosystem_build/index.html but the others are in example_docs/docs/_build/

and yes the error is weird say the size of the image is different for one pixel

MaldoAlberto commented 1 year ago

Looks good! I only have one suggestion but it's non-blocking.

If I'm not mistaken, this needs to add the announce snapshots

javabster commented 1 year ago

I adjusted the padding of the banner and made it so that users can configure the text used for the link (it defaults to "Learn more" unless specified in conf.py)

javabster commented 1 year ago

Issue created to add snapshot tests: https://github.com/Qiskit/qiskit_sphinx_theme/issues/533

atalle commented 1 year ago

@javabster I'm sorry I'm just seeing your comment in slack about padding! Can we increase top/bottom padding from 14px to 16px?