Open janogarcia opened 4 years ago
Hi Alejandro,
Thank you for reaching out and reporting this. At first glance, I think you're right. There's no need to set the icons again in the hydrateModal() method. This must be a chunk of code I overlooked while working on this. I'll take a more in-depth look at this when I get a chance and if it checks out, I'll remove those lines for the next version. Btw, your implementation of MailtoUI looks great! Love the design.
Thanks again for letting me know!
On Sun, Aug 23, 2020 at 6:54 AM Alejandro Garcรญa notifications@github.com wrote:
I may be missing something, but I'm unsure about the need to set the button icons again in hydrateModal() after those have been already set by buildModal().
I mean, the hydrateModal() method seems that should be dealing with the mailto: link data only, while the global options (e.g. the button icons) are set once at modal build time by buildModal().
https://github.com/mariordev/mailtoui/blob/v1.0.3/src/js/mailtoui.js#L360-L373
That behavior is preventing from creating proper standalone templates. While they are allowed in practice by the modalExists() check in embedModal(), the custom SVG icons embedded in the HTML template get replaced by the default ones unless you pass the custom icons in the configuration object. That won't happen for the text nodes embedded in the HTML template, which leads to a somehow inconsistent behavior.
I get that custom templates is not an officially supported feature. ๐ But even then, I'm not sure about the need to set the icons again in hydrateModal(). It seems that it should be either setting all the global options again on hydrateModal() for no specific reason or none of them, but not a subset.
Once again, sorry if I'm missing something obvious.
I removed the aforementioned lines from hydrateModal() in my fork to be able to use this custom template directly in my page without having to pass again the icons in the configuration object:
https://gist.github.com/janogarcia/54bcaacc81887b7e85340a696c7374f7
โ You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mariordev/mailtoui/issues/25, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACS3NSTJ3UZQ6DMA4DXAWITSCENQTANCNFSM4QIVTOAA .
Thank you for reviewing it, Mario. ๐
Glad that you liked the implementation. The stock MailtoUI implementation was already super close to the experience I had imagined for my mailto links. It's been a breeze to customize it and I'm extremely happy with the result. Thank you for having made it possible!
@janogarcia Hey loved your custom template! How can i use it ? Please include the markup with source code.Thank a lot.
I may be missing something, but I'm unsure about the need to set the button icons again in
hydrateModal()
after those have been already set bybuildModal()
.I mean, the
hydrateModal()
method seems that should be dealing with the mailto: link data only, while the global options (e.g. the button icons) are set once at modal build time bybuildModal()
.https://github.com/mariordev/mailtoui/blob/v1.0.3/src/js/mailtoui.js#L360-L373
That behavior is preventing from creating proper standalone templates. While they are allowed in practice by the
modalExists()
check inembedModal()
, the custom SVG icons embedded in the HTML template get replaced by the default ones unless you pass the custom icons in the configuration object. That won't happen for the text nodes embedded in the HTML template, which leads to a somehow inconsistent behavior.I get that custom templates is not an officially supported feature. ๐ But even then, I'm not sure about the need to set the icons again in
hydrateModal()
. It seems that it should be either setting all the global options again onhydrateModal()
for no specific reason or none of them, but not a subset.Once again, sorry if I'm missing something obvious.
I removed the aforementioned lines from
hydrateModal()
in my fork to be able to use this custom template directly in my page without having to pass again the icons in the configuration object:https://gist.github.com/janogarcia/54bcaacc81887b7e85340a696c7374f7