Skip to content
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

Feature/moderation #79

Merged
merged 44 commits into from
Feb 27, 2018
Merged

Feature/moderation #79

merged 44 commits into from
Feb 27, 2018

Conversation

NGambini
Copy link
Contributor

No description provided.

wopata-ngambini and others added 30 commits November 15, 2017 23:18
Release 0.8.0 - Minor bug fixes, host image on DockerHub and add dev configs for 0.8 API
@NGambini NGambini requested a review from Betree February 11, 2018 17:09
<div className="container">
<h1 className="title is-1 has-text-centered">{t('title')}</h1>
{items && items.map(el =>
<ModerationEntry key={el.id} entry={el} onAction={this.onEntryAction.bind(this)}></ModerationEntry>)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

j'ai un doute sur le bind(this), il n'y a pas plus simple ?

Copy link
Member

Choose a reason for hiding this comment

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

Les deux approches possibles :

  • Quand la fonction n'est appelée qu'à un seul endroit :
onAction={(e, a) => this.onEntryAction(e, a)}
  • Sinon le bind dans le constructor pour ne le faire qu'une fois :
constructor(props) {
  super(props)
  this.onEntryAction = this.onEntryAction.bind(this)
}

Mais ton approche fonctionne, c'est pas un problème

margin: 0 10%
&-button
width: 25%
&>.icon
Copy link
Contributor Author

Choose a reason for hiding this comment

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

je ne comprends pas pourquoi je suis obligé de faire ca, une règle globale de bulma applique des marges aux icones et les fait passer sous le texte, pourtant tu n'as pas ce problème ailleurs

Copy link
Member

Choose a reason for hiding this comment

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

Il faut mettre ton texte dans un span

<button class="moderation-entry-button button is-danger">
    <span class="icon "><i class="fa icon-close"></i></span>
    <span>Marquer comme abusif</span> // <==== Ici
</button>

@@ -0,0 +1,5 @@
{
"compilerOptions": {
"experimentalDecorators": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai du ajouter ceci pour corriger une erreur de linting de mon éditeur, dis moi si tu veux que je le mette dans mon gitignore

Copy link
Member

Choose a reason for hiding this comment

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

Nop très bien ça servira aux prochains

brunch-config.js Outdated
@@ -19,7 +19,7 @@ module.exports = {
plugins: ["transform-decorators-legacy", "transform-class-properties"]
},
sass: {
mode: 'native',
mode: 'ruby',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oups

Copy link
Member

Choose a reason for hiding this comment

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

Travis aime pas ça ;)

@Betree
Copy link
Member

Betree commented Feb 11, 2018

Merci pour la PR je test ça dans la journée 😉

Copy link
Member

@Betree Betree left a comment

Choose a reason for hiding this comment

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

Il manque une partie importante: afficher le texte du commentaire et / ou la source pour savoir ce qu'on est en train de modérer. Je viens de check l'API et c'est pas enregistré dans le changes comme ça devrait l'être. Je fais une MAJ dans la journée pour fixer ça et tu pourras y accéder dans action.changes

@@ -135,6 +135,11 @@ export default class Sidebar extends React.PureComponent {

<p className="menu-label">{ t('menu.other') }</p>
<ul className="menu-list">
<ReputationGuard requiredRep={MODERATION_REPUTATION_REQUIRED}>
Copy link
Member

Choose a reason for hiding this comment

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

Ici il faudrait masquer l'entrée s'il n'y a pas assez de réputation.

selection_203

Copy link
Member

Choose a reason for hiding this comment

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

On peut également mettre l'entrée dans la section "Contenu"

margin: 0 10%
&-button
width: 25%
&>.icon
Copy link
Member

Choose a reason for hiding this comment

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

Il faut mettre ton texte dans un span

<button class="moderation-entry-button button is-danger">
    <span class="icon "><i class="fa icon-close"></i></span>
    <span>Marquer comme abusif</span> // <==== Ici
</button>

},
"actions": {
"flag_abusive": "Marquer comme abusif",
"unsure": "Pas sur",
Copy link
Member

Choose a reason for hiding this comment

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

sûr

const { isLoading, hasReputation, t } = this.props

if (isLoading)
return <LoadingFrame/>
Copy link
Member

Choose a reason for hiding this comment

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

Pendant le chargement la sidebar affiche :
selection_205

Il faut un moyen de juste masquer totalement un item sans message si c'est spécifié

Copy link
Contributor Author

Choose a reason for hiding this comment

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

j'ai fait :

            {this.props.canAccessModeration ? <this.MenuListLink to="/moderation" iconName="flag" className="hide-when-collapsed">
              { t('menu.moderation') }

Au début je voulais ajouter le controle de la réputation requise au composant MenuLink. Mais il n'a pas accès au store, d'ailleurs je trouve ca bizarre ta façon de déclarer des composants "internes" je trouve ca pas clair la façon dont l'objet this est passé au fils

Copy link
Member

Choose a reason for hiding this comment

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

Ton dumb component peut être une simple fonction qui retourne un composant. Ici c'est le cas,

<this.MenuListLink x="42"/>

Sera compilé par React en :

React.createElement(this.MenuListLink(), {x: "42"});

L'objet this n'est pas directement passé au fils, c'est juste que c'est une méthode et pas une fonction.

Sinon plutôt que dupliquer le code de contrôle de la vérification, tu peux accepter un paramètre hideWhenNotAllowed dans le ReputationGuard et retourner null quand il n'y a pas assez de réputation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Voilà c'est fait

@NGambini
Copy link
Contributor Author

Concernant ton message :

Je viens de check l'API et c'est pas enregistré dans le changes comme ça devrait l'être. Je fais une MAJ dans la journée pour fixer ça et tu pourras y accéder dans action.changes.

C'est fait ? on est d'accord que je n'ai rien a changer, c'est déja géré par UserAction ?

@Betree
Copy link
Member

Betree commented Feb 15, 2018

L'API dev est mise à jour oui. UserAction ne gère pas l'affichage des valeurs, on va s'inspirer ou utiliser ActionDiff pour ça. Je dois faire des checks pour faire en sorte que ça marche avec l'ancien format de l'API comme avec le nouveau, c'est ok pour toi si je prend la main à partir de là ?

return <LoadingFrame/>
if (hasReputation)
return this.props.children
return <div>{ t('client.notEnoughReputation') }</div>
return showNotEnough ? <div>{ t('client.notEnoughReputation') }</div> : ''
Copy link
Member

@Betree Betree Feb 20, 2018

Choose a reason for hiding this comment

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

Plutôt qu'une chaîne vide on retourne en général null, React comprend qu'il ne doit rien ajouter au DOM

@Betree
Copy link
Member

Betree commented Feb 20, 2018

@NGambini merci pour le commit 😉 je vais check pour la compatibilité avec les deux versions de l'API et ça part dans la prochaine release !

@Betree
Copy link
Member

Betree commented Feb 27, 2018

Implement #24

selection_248
selection_246
selection_244

@Betree Betree changed the base branch from master to staging February 27, 2018 05:24
@Betree Betree merged commit e643352 into staging Feb 27, 2018
@Betree Betree mentioned this pull request Feb 27, 2018
@Betree Betree deleted the feature/moderation branch March 23, 2018 07:41
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.

3 participants