kas-catholic / confessit-web

Source code for https://confessit.app
MIT License
18 stars 7 forks source link

coherence with principles stated in #63 #67

Closed JohnRDOrazio closed 11 months ago

JohnRDOrazio commented 11 months ago

Just to keep all Trans components nice and tidy. I just noticed that the Trans components in Prayers.js have nested elements: this won't work when automatically scanning such Trans components. So simply for coherence with our non-nested elements principle, so as to avoid indexed nodes being generated on a scan, this nests the Trans components in <p>s rather than the <p>s within the Trans components.

netlify[bot] commented 11 months ago

Deploy Preview for confessit-web ready!

Name Link
Latest commit 00b73e3bc9dadf3b1f01c8fe8dcb6d9fafe1c470
Latest deploy log https://app.netlify.com/sites/confessit-web/deploys/6476682fcddf830008ebf58e
Deploy Preview https://deploy-preview-67--confessit-web.netlify.app/
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

JohnRDOrazio commented 11 months ago

I hope you find commit 4053d02 acceptable, I figure it's useless to translate the exact same text twice in two different contexts. I think it should be acceptable to recycle the translation from Prayers.js...

kas-catholic commented 11 months ago

Re-reading your description, I think I get it now. The important thing is to change <Trans><p>...</p></Trans> to <p><Trans></Trans></p>. And the problem with the nested tags is having <p>...<br />...</p> inside a translation. That makes sense to me, let's do that!

On my first reading I thought we had <Trans><p>...</p><p>...</p></Trans> but I see now that's not the case.

:+1: to merge after fixing the act of contrition key!

JohnRDOrazio commented 11 months ago

Re-reading your description, I think I get it now. The important thing is to change <Trans><p>...</p></Trans> to <p><Trans></Trans></p>. And the problem with the nested tags is having <p>...<br />...</p> inside a translation. That makes sense to me, let's do that!

On my first reading I thought we had <Trans><p>...</p><p>...</p></Trans> but I see now that's not the case.

👍 to merge after fixing the act of contrition key!

Exactly! Even though creating nested tags manually seemed to work in the translations, it won't work if we start implementing automated scanners. The scanners will treat them as indexed tags. So if at any point we were to re-scan all Trans components, at least these won't be nested and won't result in indexed tags.

JohnRDOrazio commented 11 months ago

Requesting changes because of the incorrect translation key for the act of contrition.

I'm fine merging this after that's fixed, but I don't understand why we need to. In #63, you say

Any nodes within Trans components that have attributes, or are nested, or do not have just text nodes as children, will be converted to indexed nodes

I believe you got that from these docs. But my reading of the linked docs is that a component like this <Trans><p>foo</p><p>bar</p></Trans> is not nested according to those same linked docs. Indeed, <p> is listed as an example under "elements that will be readable in translation strings", and it was readable as <p> in the existing translations. It is included in the default setting of transKeepBasicHtmlNodesFor.

Am I missing something about why its important that we change all these <p> tags to <br />? <p> is preferable from a css/styling perspective...

Only change there was in the Italian: I had originally used the most common version of the act of contrition in Walkthrough.js, and two other versions in Prayers.js. But since I removed the Trans component with the act of contrition from Walkthrough.js, I figured it would probably be a good idea to keep the most common version in any case, so I substituted the first one in Prayers.js with the most common one, and the second alternate version with the second most common version offered by the Ritual in Italian. Third alternate version in Italian wound up getting ditched in this process.

JohnRDOrazio commented 11 months ago

And in fact the whole reason I even noticed any of this was when doing another run with i18n-scanner, and seeing that it still output indexed tags for certain Trans components