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

Importe l’icono SVG inline #38

Merged
merged 5 commits into from Feb 27, 2019
Merged

Importe l’icono SVG inline #38

merged 5 commits into from Feb 27, 2019

Conversation

ariasuni
Copy link
Collaborator

Fix #3

@@ -51,62 +64,62 @@ export const urls = {
// pictos
[INSERTION_PICTO]: {
"production": undefined,
"demo": 'https://cdn.rawgit.com/datalocale/pictoGironde/master/Insertion.svg',
"demo": insertionSvg,
Copy link
Member

Choose a reason for hiding this comment

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

Si les SVG sont versionnés et inlinés, je pense que le système d'URL par NODE_ENV n'est plus nécessaire pour les SVG

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Du coup je fais juste [INSERTION_PICTO]: insertionSvg,?

Copy link
Member

Choose a reason for hiding this comment

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

C'est une option

Une autre option, c'est de supprimer ces lignes et les constantes (INSERTION_PICTO) et d'importer directement les SVG dans les composants qui en ont besoin

Une autre option, c'est de se dire "whatev, c'est pas le sujet, je fais une issue pour ne pas oublier, c'est un travail pour une autre fois"

Copy link
Member

Choose a reason for hiding this comment

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

Je te laisse choisir l'option que tu préfères selon ton humeur :-)

@@ -17,7 +22,8 @@ import {EXPENDITURES} from '../../../../shared/js/finance/constants';
import DownloadSection from "../../../../shared/js/components/gironde.fr/DownloadSection";


import {assets, INSERTION_PICTO, ENFANCE_PICTO, HANDICAPES_PICTO, PERSONNES_AGEES_PICTO} from '../../constants/resources';
import assets from '../../constants/resources';

Choose a reason for hiding this comment

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

La variable assets est encore utilisée ? C'était un export nommé avant. Et je n'ai plus l'impression que la variable serve encore dans ce fichier.

Copy link
Collaborator Author

@ariasuni ariasuni Feb 22, 2019

Choose a reason for hiding this comment

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

C’est utilisé plus bas dans le fichier car assets contient les URL vers les données financières et les textes en CSV. On peut le changer mais ça me semble hors de la portée de cette PR.

@thom4parisot
Copy link

J'ai ajouté un commentaire, c'est bon pour moi après ça 👍

@thom4parisot thom4parisot added this to In progress in Itération 3 Feb 26, 2019
@thom4parisot thom4parisot removed this from In progress in Itération 3 Feb 26, 2019
@ariasuni
Copy link
Collaborator Author

On peut fusionner ou je dois faire des modifs?

@thom4parisot
Copy link

Si tu résous le conflit, c'est bon pour être fusionné 👍

@thom4parisot thom4parisot self-requested a review February 27, 2019 16:08
@ariasuni ariasuni merged commit 30eeaa0 into master Feb 27, 2019
@ariasuni ariasuni deleted the issue3 branch February 27, 2019 21:34
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

3 participants