-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementing hyva compatibility #357
Implementing hyva compatibility #357
Conversation
9c51eed
to
d89c34e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just review a small comment. Everything else LGTM, excellent job!! 💪
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "doofinder/doofinder-magento2", | |||
"version": "0.14.13", | |||
"version": "1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that this PR contains breaking changes to a degree that you have to declare it as a major version change? 😱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jaja, realmente no debería haber ningún breaking change, solamente "fixing" changes. Era porque estaba comentando con @sofia-doofinder que en algún momento podríamos poner la 1.0.0 y habíamos comentado que quizá era un bueno momento.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo me esperaría a una más grande, pero bueno, como el código funciona lo apruebo 😄
<page xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:noNamespaceSchemaLocation="urn:magento:framework:View/Layout/etc/page_configuration.xsd"> | ||
<head> | ||
<!-- For hyva themes, jQuery can't be used among other modifications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent approach!! 🏆
@@ -0,0 +1,21 @@ | |||
<?xml version="1.0"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ogomezba una pregunta, esto así se importa sólo cuando se detecta que existe el tema hyva?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hola @sofia-doofinder ! Sí, justo lo comentaba en la descripción del PR. Según comentan en la documentación de hyva aquí, una de las cosas que hace el tema es crear layout handles por cada uno de los que ya se tengan pero añadiendo el hyva_ delante. Aunque mi conocimiento de los detalles de Magento es el que es, por lo que he entendido, los handles vienen a ser los layout.xml que se usan y se calculan matcheando la ruta con el nombre del archivo layout (excepto el default que parece que afecta a todos). Entonces, como se matchea por nombre, al llamarse el archivo hyva_default.xml, en temas normales no va a matchear nada por lo que no se carga. No obstante, el tema hyva parece que modifica este comportamiento y matchea también "hyva_lo_que_toque_por_ruta.xml". Entonces, para temas hyva sí que se añade.
De todas formas, he probado este comportamiento en la tienda del cliente con hyva y en mi tienda local sin hyva y he visto que se comportaba así.
d89c34e
to
0909038
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great! 🚀
Notas de la solución:
hyva_
. Con esto y usando la opción de remove de los layouts, conseguimos renderizar únicamente el script correspondiente en función de si está instalado o no el tema hyva.ajax()
de jQuery pero sí lo es cuando usamosfetch()
yjson()
para procesar la respuesta. Por esto, se unifica lo que devolvemos para siempre devolver un JSON.