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: Add outliner module #68

Closed
wants to merge 9 commits into from
Closed

Conversation

florianess
Copy link
Contributor

No description provided.

@benel
Copy link
Member

benel commented Jan 16, 2018

Merci Florian.

Avant de vous dire ce que vous pourriez éditer sans trop de peine dans l'historique, je vais vous demander de faire le jeu sérieux suivant pour vous entrainer : https://learngitbranching.js.org

Copy link
Member

@benel benel left a comment

Choose a reason for hiding this comment

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

Merci pour votre contribution et à très bientôt pour l'intégration de sa version amendée.

@@ -0,0 +1,7858 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Je sais bien que React conseille de "versionner" ce fichier mais voilà pourquoi on ne le fera pas :
l'un des enjeux pour nous de passer à React est de réduire le nombre de lignes (par rapport à Porphyry v6 et LaSuli v2). Si on ajoute 8200 lignes rien que pour un fichier généré de manière automatique on perd clairement cet avantage.

<h3>
{this.props.viewpoint.name}
<a className='outliner' href={outliner}>
<img src='https://static.xx.fbcdn.net/rsrc.php/v3/y9/r/tlXhSXComXE.png'/>
Copy link
Member

Choose a reason for hiding this comment

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

Il est problématique de référencer des ressources externes surtout quand elles sont hors de notre contrôle :

  • elles sont inaccessibles en mode déconnecté (par exemple si on contribue au développement dans un train),
  • elles pourraient disparaître du jour au lendemain,
  • elles pourraient être remplacées par des ressources vérolées ou par des appels à des services tiers (cf. CSRF).

Vous pouvez remplacer cette ressource :

  • par un emoji (ex : ✏️📝🖋✎) – ce qui est plus léger, mais ne fonctionnera peut-être pas avec tous les navigateurs,
  • par une icone – sous licence libre (ex : https://github.com/thesabbir/simple-line-icons/) – intégrée au projet.

Copy link
Member

Choose a reason for hiding this comment

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

J'ai oublié de l'indiquer, mais vous avez le même problème à deux autres endroits avec l'icone de suppression.

@@ -1,7 +1,7 @@
{
"user": "aaf",
"user": "vitraux",
Copy link
Member

Choose a reason for hiding this comment

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

Si vous n'avez pas de raison claire de le faire, ne changez pas les paramètres de configuration qui sont donnés dans un projet auquel vous contribuez.

"services": [
"http://argos2.hypertopic.org",
"http://argos2.test.hypertopic.org",
Copy link
Member

Choose a reason for hiding this comment

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

Idem.

@@ -1,126 +0,0 @@
import React from "react";
Copy link
Member

Choose a reason for hiding this comment

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

Évitez ce genre de commit.
Si le contenu supprimé correspondait à une phase signifiante de votre projet, combinez dans un même commit la suppression et ce que vous avez ajouté à la place. Le commit aura alors pour nom REFACTORING: suivi de la raison d'être de cette réécriture.

@gumn gumn mentioned this pull request May 11, 2018
4 tasks
@benel
Copy link
Member

benel commented Jun 8, 2018

Currently being integrated and modified by IF05 teams.

@benel benel closed this Jun 8, 2018
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.

None yet

2 participants