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 - Brad McKenzie #75

Merged
merged 4 commits into from
Jan 15, 2025
Merged

Project - Brad McKenzie #75

merged 4 commits into from
Jan 15, 2025

Conversation

bm-go
Copy link
Contributor

@bm-go bm-go commented Nov 11, 2024

Initial request to setup dataviz final project

Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for mucss-dataviz ready!

Name Link
🔨 Latest commit f1ff23b
🔍 Latest deploy log https://app.netlify.com/sites/mucss-dataviz/deploys/6787f19ffa74270008bc6a2a
😎 Deploy Preview https://deploy-preview-75--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.

@Enchufa2 Enchufa2 marked this pull request as draft November 14, 2024 12:15
Copy link
Contributor Author

@bm-go bm-go left a comment

Choose a reason for hiding this comment

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

Hi Iñaki, the initial version of my project is ready for review!

@Enchufa2 Enchufa2 marked this pull request as ready for review January 9, 2025 18:07
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.

_projects/2024/100535241/100535241.Rmd Outdated Show resolved Hide resolved
_projects/2024/100535241/100535241.Rmd Outdated Show resolved Hide resolved
_projects/2024/100535241/100535241.Rmd Outdated Show resolved Hide resolved
_projects/2024/100535241/100535241.Rmd Outdated Show resolved Hide resolved
_projects/2024/100535241/100535241.Rmd Outdated Show resolved Hide resolved
_projects/2024/100535241/100535241.Rmd Outdated Show resolved Hide resolved
_projects/2024/100535241/100535241.Rmd Outdated Show resolved Hide resolved
_projects/2024/100535241/100535241.Rmd Outdated Show resolved Hide resolved
@Enchufa2
Copy link
Member

Enchufa2 commented Jan 9, 2025

Also, please add preview = TRUE in the chunk options for the chunk producing the final replication, so that this image is shown in the projects list.

@Enchufa2
Copy link
Member

Just a kind reminder that the deadline is tomorrow. :)

@bm-go
Copy link
Contributor Author

bm-go commented Jan 14, 2025

Hello! All comments should now be addressed. I have committed changes and pushed back for any final checks and changes!

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.

Nice, thanks! Merging after adding the resulting images too.

@Enchufa2 Enchufa2 merged commit 6a3e0b9 into IBiDat:main Jan 15, 2025
4 checks passed
@Enchufa2
Copy link
Member

Here's your post, congrats! https://csslab.uc3m.es/dataviz/projects/2024/100535241/

Copy link
Contributor Author

@bm-go bm-go left a comment

Choose a reason for hiding this comment

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

Found an error on the publication with the image final plot image generation, may need to push/pull my own figure-html5 folder?

@Enchufa2
Copy link
Member

Ah, let me see if I can fix it.

@Enchufa2
Copy link
Member

Better now? The limits of the inset were... odd. I've also removed the need for saving the legend in the first place.

@bm-go
Copy link
Contributor Author

bm-go commented Jan 15, 2025

Thanks for the removal of the image save, more efficient.

Your changes look sensible, but it has cut off the last few letters of the legend and changed the sizing a bit. I'm not too worried about the exactness of it. But just to ensure it's all at least readable, can you update these lines of code? I don't think I can do it now that it's published to your main repos.

Line 458: (to expand legend width to show the full labels)
Current: lims(x=c(-3,20))+
Update to: lims(x=c(-3,21))+

Lines 489-493: (slight realignment/sizing)
Current: inset_element(concentric_plot,
left=0.02,
bottom=0.96,
right=0.32,
top=1.005,
Update to: inset_element(concentric_plot,
left=0.01,
bottom=0.955,
right=0.4,
top=1.01,

Apologies for the minor adjustments. Hopefully my checks line up and this finalises it!!!

@Enchufa2
Copy link
Member

Ok, done!

@bm-go
Copy link
Contributor Author

bm-go commented Jan 16, 2025

Thank you! Fixed :)

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