Fix display of local announcement banner #1822
Conversation
Coverage report
This PR does not seem to contain any modification to coverable code. |
| if (is_remote) { | ||
| document.write(`<aside class="bd-header-announcement d-print-none" aria-label="{{ banner_label }}"></aside>`); | ||
| fetch("{{ theme_announcement }}") | ||
| .then(res => {return res.text();}) | ||
| .then(data => { | ||
| if (data.length === 0) { | ||
| console.log("[PST]: Empty announcement at: {{ theme_announcement }}"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
If I'm reading this correctly, the aside tag will get written first, even if data.length === 0. Seems like we wouldn't want that to happen?
There was a problem hiding this comment.
oh good point, let me fix it
There was a problem hiding this comment.
By the way, PR 1808 has a test case that checks that the banner gets rendered when theme announcement is local.
As an aside, one reason I was holding off on 1808 was that I wanted to see if someone had any thoughts about how to write the tests I added in a more efficient way, but that could always be done in a follow-up PR.
There was a problem hiding this comment.
Oh - sorry about that, I should have checked. I'll pause here and let you finish 1808 first, thanks!
There was a problem hiding this comment.
No worries, I think it's awesome you jumped in to help fix the problem :)
There was a problem hiding this comment.
@melissawm, I edited some updates you made to the docs about the theme announcement, so if you could double check that my rewrite is accurate that would be awesome
|
Closing in favor of #1808, thanks all. |
Closes #1817
A small refactor of the announcement banner display logic to fix the display of local announcement banners.