Fix newsletters unwanted CSS and 404 page on preview#9635
Fix newsletters unwanted CSS and 404 page on preview#9635alecslupu merged 15 commits intodecidim:developfrom
Conversation
|
Let's retake this PR @ahukkanen, can you merge with develop 🙏🏽 ? Thanks |
|
Done @andreslucena |
andreslucena
left a comment
There was a problem hiding this comment.
Related to the preview links, I think we should also fix the links to the organization page (root path/homepage) that are in the header and footer, as now they're buggy:
andreslucena
left a comment
There was a problem hiding this comment.
Unwanted CSS
Confirmed locally. Great fix and finally a spec for this one 👏🏽 👏🏽
404 page on preview
Related to the preview links, I think we should also fix the links to the organization page (root path/homepage) that are in the header and footer, as now they're buggy:
-
By default, on development, they're pointing to
http://localhost, and as we're actually by default serving the development web server at http://localhost:3000, this gives a 503 error -
If I change it to http://localhost:3000, then the homepage opens in the iframe
You can see both of these behaviors with the video:
newsletter-logo_footer-homepage.mp4
--
We could skip these and also the "notifications" settings link, but I'd prefer to have all the fixes in the same PR so its easier to backport. What do you think?
|
Both link URLs are now fixed in the latest commits during the preview. |
decidim-core/app/views/decidim/newsletter_mailer/newsletter.html.erb
Outdated
Show resolved
Hide resolved
alecslupu
left a comment
There was a problem hiding this comment.
Hello @ahukkanen ,
While reviewing this, i have noticed, that the sent newsletter has the wrong url on the organization links.
Testing the NewsletterTe...
Can’t see this email correctly? View it on the website
( http://alec****u.g*.ro:3000/newsletters/6 ).
Hand-Barton
( http://alec****u.g*.ro/?utm_source=alec****u.g*.ro&utm_campaign=6 )
Testing the NewsletterTesting the NewsletterTesting
the NewsletterTesting the NewsletterTesting the NewsletterTesting
the NewsletterTesting the NewsletterTesting the NewsletterTesting
the NewsletterTesting the NewsletterTesting the NewsletterTesting
the NewsletterTesting the NewsletterTesting the NewsletterTesting
the Newsletter
This is also valid for the footer organization link.
I could not verify the fix for : #8727, as the not sent newsletter is still being redirected 404 by
Regarding The CSS line, I can confirm is being correctly rendered.
<style>
table.button table td {
background: #ef604d !important
}
</style>
The requested changes are referring to organization url .
I can also confirm that if the instance has the following
config.action_mailer.default_url_options = { port: 3300 }
config.action_controller.default_url_options = { port: 3300 }
All the links are being generated as it should.
Thanks
|
@alecslupu Those issues should be fixed by the latest commits. |
alecslupu
left a comment
There was a problem hiding this comment.
The preview issues are fixed now.
Tested locally, and works as expected with few changes afetr this review. Dismissing to allow me to merge current PR.
🎩 What? Why?
When using the image with button text and the organization has a primary color defined, the "view on web" template currently shows the CSS as plain text at the beginning of the message.
This is a persistent issue that has been fixed a couple of times before in different situations, see the related issues.
Another issue this fixes is the 404 links in the newsletter previews.
📌 Related Issues
Testing
#which means they open the same page (I found this better than disabling the links as these links are coming from the translations and I did not want to add any JS to the newsletter template views)