Skip to content

chore(embedLink): new Layout#2

Merged
Dindonneau merged 3 commits intodevfrom
newEmbedLinkLayout
Dec 21, 2023
Merged

chore(embedLink): new Layout#2
Dindonneau merged 3 commits intodevfrom
newEmbedLinkLayout

Conversation

@Dindonneau
Copy link
Copy Markdown

J'ai laissé en commentaire l'ancien code au cas où

@Dindonneau Dindonneau requested a review from antoineol December 21, 2023 15:58
src/index.css Outdated
clear: both;
display: table;
}
} */
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tu as vraiment besoin de garder ce code commenté ?

Pareil pour tous les autres.

Principe : Ne garde pas de code mort, sauf si tu as une bonne raison (qu'il vaut mieux expliquer en commentaire)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dans le commentaire de PR, je précisais que je laissais l'ancien code en commentaire au cas où. En cas de problème: on a l'ancien code devant les yeux, donc on peut facilement le remettre: si aucun soucis de comportement constaté à l'usage, on pourra l'enlever sereinement.

src/index.js Outdated
this.nodes.inputHolder.remove();
this.showLinkPreview(metaData);
this.showLinkPreviewOverridden(metaData);
// this.showLinkPreview(metaData);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qu'est devenu showLinkPreview ? Il est utilisé ailleurs ? Faut-il le garder ? Si c'est à supprimer, pourquoi avoir fait une 2è méthode "Overridden" au lieu de modifier la méthode d'origine ?

Sur la méthode Overridden, si j'ai bien compris, tu as juste enlevé les 3 dernières lignes ? En supposant qu'il soit utile de garder la méthode d'origine (usage ailleurs), pourquoi ne pas avoir juste split en 2 la méthode, pour éviter ce gros tas de code dupliqué ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Il n'y a pas que les 3 dernières lignes qui changent étant donné que je crée un nouveau container dans lequel je place titre, description et link.

J'ai gardé la première méthode au cas où on constaterait plus tard un comportement inattendu malgré les tests que j'ai pu faire.

@Dindonneau Dindonneau merged commit f855711 into dev Dec 21, 2023
@Dindonneau Dindonneau deleted the newEmbedLinkLayout branch December 21, 2023 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants