Group English download links together#1315
Conversation
Previously the English RTF "translation" was included with the other translations. This commit gets the English RTF translation and puts it right under the original English PDF link.
Also use append to set text so we don't lose icon using innerHTML, same for non-RTF branch since it's safer anyway to not parse HTML if we don't need to
xmedr
left a comment
xmedr
left a comment
There was a problem hiding this comment.
Solid stuff! I've got a few suggestions/questons below, but the app works as expected and looks great.
Thanks for adding js tests to this, especially for agendas on the index since that's harder to set up! I think adding jest to CI is a great idea! Would you be able to open an issue for that in this repo?
Also, the 61 you have linked in your Overview is a councilmatic specific issue haha. Try using just the straight up link to an issue when it's in a different repo. Here's the link for translation's issue 61: datamade/la-metro-translations#61
| // Special treatment for English RTF | ||
| // Languages are ordered so English will always be first | ||
| if (file_format === "rtf") { | ||
| const [engFile] = linksArr.splice(0,1) |
There was a problem hiding this comment.
It looks like this line exists to remove the first element of the linksArr and put it into engFile, is that right?
If so, would Array.shift() serve the same purpose and make this a bit easier to read? Maybe something like:
const engFile = linksArr.shift()
| icon.classList.add("fa") | ||
| icon.classList.add("fa-file-text-o") |
There was a problem hiding this comment.
We can consolidate these into one .add()!
| icon.classList.add("fa") | |
| icon.classList.add("fa-file-text-o") | |
| icon.classList.add("fa", "fa-file-text-o") |
| <div> | ||
| <strong> | ||
| <a href='{{board_report.url}}' target="_blank"> | ||
| <i class="fa fa-file-text-o" aria-hidden="true"></i> Download Board Report (English) |
There was a problem hiding this comment.
Think this might also need to have PDF at the end similar to the event template
| <strong> | ||
| <a href='{{agenda.url}}' target="_blank"> | ||
| <i class="fa fa-file-text-o" aria-hidden="true"></i> Download Agenda (English) | ||
| <i class="fa fa-file-text-o" aria-hidden="true"></i> Download Agenda (English) [pdf] |
There was a problem hiding this comment.
I know Neil had this as lowercase in his screenshot, but I'm realizing we may want to standardize whether we're using PDF/RTF or pdf/rtf on event and bill detail pages. Let's maybe go with making them uppercase, since text elsewhere on the page has PDF written that way. So something like:
| @@ -0,0 +1,144 @@ | |||
| const { IndexTranslationUtils, DetailPageTranslationUtils } = require("./utils") | |||
There was a problem hiding this comment.
I'm currently getting an error after installing and running jest. It's a bit scant on details, but do you have an idea what's going on here?
>>> npx jest
Unexpected token .3 participants
Overview
When receiving translations, splice off the first RTF translation which is always English by the ordering in the translations app (at least it is for now, maybe I should add a check?) and put it in a special placeholder div in event detail and bill detail pages.
Connects #61
Demo
Should look like this (note that these screenshots were taken from hardcoded versions of the template to show what it should look like when the English RTF div is populated by
renderLinks)Testing Instructions
npm installthennpx jestNote that
jesttesting isn't integrated into our CI workflow yet. Should it be?