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

Project - Jorge Ramos #81

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Project - Jorge Ramos #81

wants to merge 2 commits into from

Conversation

jjorgeramos
Copy link

Good Evening Iñaki,

I have already uploaded the initial version of the project.

I look forward to receiving your contribution.

Copy link

netlify bot commented Dec 20, 2024

Deploy Preview for mucss-dataviz ready!

Name Link
🔨 Latest commit d761f6d
🔍 Latest deploy log https://app.netlify.com/sites/mucss-dataviz/deploys/6765a6a2b2c021000880a28b
😎 Deploy Preview https://deploy-preview-81--mucss-dataviz.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jjorgeramos jjorgeramos marked this pull request as ready for review December 29, 2024 10:06
Copy link
Member

@Enchufa2 Enchufa2 left a comment

Choose a reason for hiding this comment

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

Great work! Some comments below.

toc: true
---

First I load the libraries that I will need to make the plot:
Copy link
Member

Choose a reason for hiding this comment

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

Please divide your post in sections (e.g. ## Introduction) and subsections e.g. ### Some subsection) for readability. See other posts in the gallery.

Copy link
Member

Choose a reason for hiding this comment

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

And first of all, please show the graph you selected and talk a little bit about it. See other published projects for reference.

library(showtext)
library(extrafont)
library(png)
library(devtools)
Copy link
Member

Choose a reason for hiding this comment

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

Is devtools really needed? In general, keep the number of libraries to a minimum. Just those strictly used by you directly.

Copy link
Member

Choose a reason for hiding this comment

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

Also, you only need to load the libraries once. Please remove repeated calls below.

library(flagon)
```

Now, I can start with the data i need to make the graph. In the original plot from El Orden Mundial there is one papers where they extract the information. These are: UNODC (2018): World Drug Report 2018: opioid crisis, prescription drug abuse expands; cocaine and opium hit record highs.
Copy link
Member

Choose a reason for hiding this comment

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

Check the text for typos, please (e.g. "i need"). RStudio has autocorrect tools. ;-)

Comment on lines +174 to +176
#Install the package:
library(devtools)
install_local("C:/Users/jorge/Desktop/CSS/Data-Visualization/Cocaine in Europe - Datavis/flagon-master")
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. No, it's better to avoid this. Just mention that the flagon package is avaiable only on GitHub and link the project in the text.

Comment on lines +182 to +196
Australia <- readPNG(flagon::flags("au"))
Albania <- readPNG(flagon::flags("al"))
Estados_Unidos <- readPNG(flagon::flags("us"))
Escocia <- readPNG("C:/Users/jorge/Desktop/CSS/Data-Visualization/Cocaine in Europe - Datavis/flagon-master/inst/png/sct.png")
Inglaterra <- readPNG("C:/Users/jorge/Desktop/CSS/Data-Visualization/Cocaine in Europe - Datavis/Images/England and Wales.png")
España <- readPNG(flagon::flags("es"))
Paises_Bajos <- readPNG(flagon::flags("nl"))
Irlanda_norte <- readPNG("C:/Users/jorge/Desktop/CSS/Data-Visualization/Cocaine in Europe - Datavis/Images/Northern Ireland.png")
Uruguay <- readPNG(flagon::flags("uy"))
Argentina <- readPNG(flagon::flags("ar"))
Irlanda <- readPNG(flagon::flags("ie"))
Canada <- readPNG(flagon::flags("ca"))
Polonia <- readPNG(flagon::flags("pl"))
Chile <- readPNG(flagon::flags("cl"))
CostaRica <- readPNG(flagon::flags("cr"))
Copy link
Member

Choose a reason for hiding this comment

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

You only need 3 special flags not included in flagon, right? Then remove the rest of them from the "Flags" directory. And those 3 should be just

Inglaterra <- readPNG("Flags/blablabla.png")

etc.

Comment on lines +238 to +239
logo1 <- readPNG("C:/Users/jorge/Desktop/CSS/Data-Visualization/Cocaine in Europe - Datavis/eom.png")
logo2 <- readPNG("C:/Users/jorge/Desktop/CSS/Data-Visualization/Cocaine in Europe - Datavis/cuch.png")
Copy link
Member

Choose a reason for hiding this comment

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

Please include these images (in general, any external files) in the PR close to this Rmd, then call them just by their name, without any path (note that these paths are in YOUR computer).


The next step is to introduce the logos. These original figures were difficult to find, so I decided to made a screenshot of the original plot and introduce the pictures with patchwork package.

```{r, fig.width= 8.5, fig.height= 6}
Copy link
Member

Choose a reason for hiding this comment

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

This chunk produces the final replication, right? Then please include preview = TRUE as a chunk option in order to show this in the project list.

Comment on lines +641 to +645
logo3 <- readPNG("C:/Users/jorge/Desktop/CSS/Data-Visualization/Cocaine in Europe - Datavis/tur.png")
logo3_grob <- rasterGrob(logo3, interpolate = TRUE)
logo4 <- readPNG("C:/Users/jorge/Desktop/CSS/Data-Visualization/Cocaine in Europe - Datavis/tur1.png")
logo4_grob <- rasterGrob(logo4, interpolate = TRUE)
logo5 <- readPNG("C:/Users/jorge/Desktop/CSS/Data-Visualization/Cocaine in Europe - Datavis/ray.png")
Copy link
Member

Choose a reason for hiding this comment

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

Same as before.

@jjorgeramos
Copy link
Author

jjorgeramos commented Jan 9, 2025 via email

@Enchufa2
Copy link
Member

Buenas noches Iñaki, estaba revisando las cosas que tengo que mejorar pero no acabo de entender la parte de las imágenes. You only need 3 special flags not included in flagon, right? Then remove the rest of them from the "Flags" directory. And those 3 should be just Inglaterra <- readPNG("Flags/blablabla.png")

Quería decir que has incluido imágenes de banderas en el PR, pero por otra parte cargas la mayoría de las banderas desde flagon. Entonces elimina las banderas que no se necesitan del PR. Por otro lado, las especiales no están aquí en PNG, solo veo SVG.

o esto Please include these images (in general, any external files) in the PR close to this Rmd, then call them just by their name, without any path (note that these paths are in YOUR computer). ¿Tendría que ponerlos en la carpeta del proyecto, cambiar el código y luego hacer el commit?

Sí. Esto tiene que ser replicable. Si hay rutas de tu ordenador a archivos que no están incluidos aquí, no puedo darle a knitr y replicar tu post. Todo lo que uses externo (imágenes, datos...) tiene que estar incluido en el pull request en la misma carpeta que el Rmd. Y referirte a ellos simplemente por el nombre del archivo, porque ya están junto al Rmd, no hace falta que incluyas la ruta completa al archivo.

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