Add banner feature to qiskit theme #530
Conversation
|
@javabster is only add the images in the snapshot test? |
|
@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 |
|
@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 |
|
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 😅 |
|
I found the path for the announcement is and yes the error is weird say the size of the image is different for one pixel |
| {% block announcement %} {{ theme_announcement }} {% endblock announcement %} | ||
| {% if announcement_url -%} | ||
| <a href="{{announcement_url}}" class="reference internal">Learn more |
There was a problem hiding this comment.
Can you allow setting the link text in the conf.py? Usually it's better to have the link text describe the page the user is going to.
If I'm not mistaken, this needs to add the announce snapshots |
|
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) |
There was a problem hiding this comment.
Looks good, thanks @javabster!
I've left another comment and the snapshot tests would be nice, but if time is a concern I think it's fine to merge now and make an issue to address later.
|
|
||
| // custom announcement banner with purple gradient | ||
| .custom-announcement { | ||
| top: var(--qiskit-top-nav-bar-height); |
There was a problem hiding this comment.
I think this line is unneeded, but it shouldn't break anything so this comment is non-blocking.
There was a problem hiding this comment.
ah good spot! I'll remove it
|
Issue created to add snapshot tests: #533 |
|
@javabster I'm sorry I'm just seeing your comment in slack about padding! Can we increase top/bottom padding from 14px to 16px? |
The Qiskit theme is being removed and we'll only have qiskit-ecosystem, per #577. The qiskit-ecosystem theme never supported these custom announcements from #530 - so this current code adds unnecessary noise. If we decide we do want this custom announcement feature for the Ecosystem, we can always add this code back easily thanks to Git. In the meantime, the focus is on making this codebase as small and sustainable as possible.
In this PR:
I'm struggling to figure out how to add new snapshots for this component, any help appreciated!