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

Allow setting a custom Carpentry type #585

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

Conversation

milanmlft
Copy link
Contributor

Allow setting a custom value for the carpentry field in the config.yaml of a lesson.

At the moment, if a user wants to create a course website using the Carpentries workbench but with custom styling and logos, they would have to clone both the {sandpaper} and {varnish} repos:

  • {varnish} to modify the styling and add custom logos
  • {sandpaper} to modify the which_caprentry() and which_icon_carpentry() functions so that it can pick up the alternative logos from varnish.

See for example https://github.com/HealthBioscienceIDEAS/microscopy-novice, which uses the HealthBioscienceIDEAS/sandpaper and HealthBioscienceIDEAS/varnish forks.

This is rather cumbersome, as one needs to clone the entire {sandpaper} repo and then keep maintained, just to modify two lines of code.

With this PR, a user can use any value in the carpentry: config field and it will be picked up by {sandpaper} and used to get the matching logo from {varnish}, as long as there are logo files present in the {varnish} fork being used that (exactly) match the carpentry name. So this omits the need to fork {sandpaper}, only {varnish} needs to be forked.

Copy link
Member

@tobyhodges tobyhodges left a comment

Choose a reason for hiding this comment

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

This looks good to me, thank you @milanmlft. @froggleston @ErinBecker do you think we should add a test to ensure that the newly-defined default is being captured?

@tobyhodges
Copy link
Member

@milanmlft a new release of sandpaper will be made soon, and we plan to review this one for inclusion in the next release after that. Thanks for your patience!

@ErinBecker
Copy link
Contributor

This is great @milanmlft! I've done some testing and was able to get this to work with a couple of different "logo" files (screenshots below).

Screenshot showing lesson with bicycle logo Screenshot showing lesson with ice cream logo

I've put in a PR with an update to the documentation for this feature: carpentries/sandpaper-docs#197 and will wait to merge this PR in until that documentation has been reviewed.

R/utils.R Show resolved Hide resolved
@milanmlft
Copy link
Contributor Author

I think this should do it. Let me know if it needs some more documentation or testing!

ErinBecker added a commit to carpentries/sandpaper-docs that referenced this pull request Jun 25, 2024
@ErinBecker
Copy link
Contributor

Thanks @milanmlft for making those updates so quickly! And thank you @tobyhodges and @froggleston for noticing this issue and suggesting a fix. I've tested to confirm that adding "carpentry_description" overrides the default alt text for the logo file (screenshots below). I've also made updates to carpentries/sandpaper-docs#197 to capture these changes.

Screenshot showing alt text from carpentry_description Screenshot showing bicycle logo

Following updates in carpentries/sandpaper-docs#197

Co-authored-by: Erin Becker <[email protected]>
@tobyhodges
Copy link
Member

Thanks @milanmlft, I really appreciate your quick and thoughtful responses to feedback, and of course the effort you put in to make the contribution.

@ErinBecker and @froggleston I'm happy if you're happy!

@milanmlft
Copy link
Contributor Author

@ErinBecker @froggleston any updates on this? Thanks!

@froggleston
Copy link
Contributor

Hi @milanmlft - as we're running on low capacity in the Core Team, we try hard to make, review and merge in updates when we can. Be sure we will get to this when we're able! As this is looking fairly complete, I would presume this would be in the next sandpaper release, which may be next week or the week after.

@carpentries carpentries deleted a comment from rokups Jul 5, 2024
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

4 participants