[WIP] Ajout d'une interface de merge pour les contenus #4151
Conversation
zds/tutorialv2/forms.py
Outdated
| else: | ||
| self.helper.layout.append(Layout(Field('introduction', css_class='md-editor'))) | ||
|
|
||
| if kwargs.get('data') is not None: |
There was a problem hiding this comment.
il te manque un , None pour forcer le get à te donner une valeur par défaut.
zds/tutorialv2/forms.py
Outdated
| Field('conclusion', css_class='md-editor'), | ||
| Field('image')) | ||
|
|
||
| if kwargs.get('data') is not None: |
There was a problem hiding this comment.
pourquoi la condition apparaît-elle deux fois?
assets/js/auto-merge.js
Outdated
| setValue(left.html()); | ||
| }, | ||
| rhs: function(setValue) { | ||
| setValue(right.text()); |
There was a problem hiding this comment.
peux-tu mettre un commentaire dans le code pour expliquer pourquoi on a "html" d'un côté et "text" de l'autre stp?
There was a problem hiding this comment.
Bien vu, j'ai uniformisé car il n'y avait pas de raison en particulier.
|
Mmh. Ça a peut être rien à voir, mais je coince à la première étape : (en plus, ça mentionne pas ton nouveau package, donc c'est décidément moi qui me plante quelque part) |
|
Si codemirror est un nouveau package, c'est une dépendance de mergely. J'ai peut être oublié quelques chose de mon côté. Si tu veut tester en contournant le problème tu peut faire un nom install mergely ce qui installera tout, le temps que je trouve une solution. |
|
il faut le mettre dans le fichier package.json non?
Le 12/01/2017 à 20:49, Anto59290 a écrit :
…
Si codemirror est un nouveau package, c'est une dépendance de mergely.
J'ai peut être oublié quelques chose de mon côté. Si tu veut tester en
contournant le problème tu peut faire un nom install mergely ce qui
installera tout, le temps que je trouve une solution.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#4151 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABc_xY76Vn2-u1DiE2RpjQQuNQQ0h4VDks5rRoO0gaJpZM4LiHK3>.
|
|
Exact. |
assets/js/auto-merge.js
Outdated
| /** | ||
| * Sets up the merge interface (using mergely). | ||
| */ | ||
| function mergelySetUp(div, left, right) |
There was a problem hiding this comment.
peux-tu mettre un $ devant chaque variable (car c'est du jQuery).
De même, peux-tu changer le nom pour que ça soit plus proche du sens "fonctionnel" c'est à dire $local et $remote au lieu de left et right. du moins je pense (@vhf tu en dis quoi?)
There was a problem hiding this comment.
Pourquoi pas changer le nom. Après local et remote c'est une convention pour nous (car le code actuel écrase), par sur que ça soit clair nom plus... Je vais détailler les commentaires avant pour expliquer les paramètres.
There was a problem hiding this comment.
Effectivement les variables jQuery doivent être préfixées par $. Je te fais vite un exemple :
var $lala = $('.foo').find('.bar').get(1);
var baz = $lala.val();Je sais pas comment nommer idéalement left et right. Si c'est côté JS qu'on charge les contenus à afficher à gauche et à droite, c'est côté JS qu'on sait quelle version risque d'écraser quelle version, et c'est sur ça qu'on devrait se baser pour nommer explicitement ces paramètres. Genre au lieu de left on pourrait avoir currentVersion et au lieu de right on aurait submittedVersion. Un truc dans ce genre. Ça a du sens pour vous ?
assets/js/auto-merge.js
Outdated
| e.preventDefault(); | ||
|
|
||
| var button = $(this); | ||
| var classList = button.attr("class").split(/\s+/); |
There was a problem hiding this comment.
hum pas sûr que ça soit la méthode la plus facile d'autant qu'il existe déjà l'attribut classList (this.classList).
Ensuite plutôt qu'un simple for, je suis d'avis d'utiliser un this.classList.forEach(function(){...}/
There was a problem hiding this comment.
De mémoire .classList à un support limité (voir inexistant) avec < IE9, d'où le choix de .attr.
There was a problem hiding this comment.
on ne supporte que IE 11 et supérieur, chrome last, firefox last, safari last.
There was a problem hiding this comment.
Ca marche alors ;)
… into 3251_git_diff_2
|
Je sais pas pourquoi le linting en Js ne passe pas, c'est assez étrange:
|
|
@Anto59290 C’est normal que jshint râle là-dessus. Utilise |
|
Je ferme mais n'hésite pas à réouvrir quand tu auras du temps pour la finir ! :) |
|
argl, a-t-on une idée de si ça reviendra un jour? |
|
👋 Au moment ou tu avais approve il restait juste de la QA et un peu de bugfix a faire il me semble. Mais dans l'ensemble tout était OK si mes souvenirs sont bons. |
|
ok, donc tu peux rouvrir et rebase stp? |
|
On en est où ici @Anto59290 ? |
Je vous propose cette fonctionnalité pour régler le problème d'écrasement lors de l'édition concurrente. Pour l'instant je ne l'ai ajouté que sur l'édition de contenu (content edit). Pour l'utiliser il faut donc éditer l'intro et/ou la conclusion d'un tuto (et non un containeur ou d'une section) depuis deux onglets différents.
C'est encore un travail en cours, mais j'aimerais votre feedback avant d'aller plus loin. La méthode proposé est normalement assez facile à dupliquer pour les autres champs. Graphiquement on est assez proche de ce que j'imaginais, il me reste juste à virer la barre violette à droite des champs. Merci d'avance de vos retours.
QA