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

Propagate _data folder from remote theme #89

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

mgerzabek
Copy link

@mgerzabek mgerzabek commented Mar 8, 2021

All files from _data folder within remote theme are accessible in consumer projects. The behaviour is exactly the same as for overrides of _includes and _layouts. Data files with the same name in consumer project override data files within remote theme.
As supposed by @hblankenship and @desima in #68


View rendered README.md

All files from _data folder within remote theme are accessible in consumer project. The behaviour is exactly the same as for overrides of _includes and _layouts. Data files with the same name in consumer project override data files within remote theme.
@welcome
Copy link

welcome bot commented Mar 8, 2021

Welcome! Congrats on your first pull request to Jekyll Remote Theme. If you haven't already, please be sure to check out the contributing guidelines.

@benbalter
Copy link
Owner

Thanks for this @mgerzabek. Can you provide a bit more detail as to the use case where a theme should be injecting data into the consumer site?

@mgerzabek
Copy link
Author

Well, I have a catalogue of text modules for a layout file concerned with GDPR. And I want them to be in the remote theme to have changes to this catalogue be available for all projects that use my theme.

Further so, I see that in putting a catalogue in a (remote) theme becoming a curated collection of possibly anything would give a better user experience/consumer experience of themes.

@datapolitical
Copy link

My current remote theme has icons stored in _data that have to be manually copied to work.

@delisma
Copy link

delisma commented Mar 12, 2021

@duboisp we could finally use the _data folder to store theme settings and variables

@mgerzabek
Copy link
Author

Since this is my first pull request and I couldn’t find a reviewer, may I ask @benbalter how the stage of this PR could be pushed forward.

As I understand from the timeline I need a review from someone with write access to this repo. Unfortunately the Reviewers section is empty, even the owner of the repo is not listed, so I cannot ask for a review. Is this some kind of a chicken-egg-related problem?

Any help/clearing words appreciated.

mgerzabek added a commit to mgerzabek/starter that referenced this pull request Apr 8, 2021
Solange der Inhalt des _data Folders aus dem remote theme nicht gelesen
werden kann, müssen die Datenschutztexte (_data/datenschutz.json) direkt
im Template Repository gehostet werden.

TODO Sobald mein PR akzeptiert wurde [1]
- _data/datenschutz.json entfernen
- .gitignore wieder den _data Folder als nicht gehostet einfügen

[1] benbalter/jekyll-remote-theme#89
@hblankenship
Copy link

hblankenship commented May 21, 2021

Another use case would be i18n files within the theme that can be accessed by the site using the theme. Just one of many, many cases. For myself, we use a remote theme for our foundation website that would store things like events which the foundation website and multiple sub-websites would then display. Our use-case is somewhat unusual:
theme
- main website
- sub-site (separate repository but 'same' website
- sub-site (as above)
- sub-site (as above)
- etc.

example case: https://owasp.org
Each project and chapter, for instance, is its own 'site'(repository) within our website but they are all linked together. If I want information to show up on each sub-site, it should reside in the theme. Please merge this.

OWASP Foundation, the Open Source Foundation for Application Security on the main website for The OWASP Foundation. OWASP is a nonprofit foundation that works to improve the security of software.

@parkr
Copy link

parkr commented Jul 23, 2021

I think this could be a really useful feature for all Jekyll themes. Would you consider submitting the change to the Jekyll project upstream?

@mgerzabek
Copy link
Author

Would you consider submitting the change to the Jekyll project upstream?

Mhm… okay, I‘m just a Jekyll end user and not a ruby pro. Beside that, of course, I’d do it, but I‘m not sure where to start. Any hints where and how to go about it?

@hblankenship
Copy link

@benbalter What do we need to do to get this PR merged? A review?

@mgerzabek
Copy link
Author

From what I understand, yes. A review from someone who can do this. Until yesterday the checks where green, then I klicked the merge button to synchronize the PR with the latest sources and now even the checks don‘t run with the message “Merging can be performed automatically with 1 approving review.“
But this is my first PR so don’t take my word for it…

@ashmaroli
Copy link
Contributor

For those interested in this feature and deploying their site using GitHub Actions,
there's a plugin that adds this feature to Jekyll: jekyll-data

…und wird dann im lokalen Repository verlinkt. Achtung! Eine Verzeichnisstruktur wie bei mir am MacBook ist erforderlich. Also alle Repositories müssen innerhalb eines git Elternverzeichnisses angelegt sein.
@mgerzabek
Copy link
Author

With the success of PR 8815 for Jekyll this PR becomes obsolete. When v4.3 is released the desired behaviour should be within Jekyll standard distro. Closing this.

@mgerzabek mgerzabek closed this Nov 22, 2021
@parkr
Copy link

parkr commented Nov 22, 2021

@mgerzabek It would be good to validate that. It's my understanding that at least the Theme class in this repo may need updating to support the data loading from the theme.

@mgerzabek
Copy link
Author

@parkr you’re right. Thanks for the hint!
I’ll check this as soon as a Jekyll release with the merged changes from the referenced PR are live.
For now I reopen this PR.

@mgerzabek mgerzabek reopened this Nov 25, 2021
@ashmaroli
Copy link
Contributor

I’ll check this as soon as a Jekyll release with the merged changes from the referenced PR are live.

@mgerzabek You need not wait for an official release.
You may point your test-site's Gemfile to the Jekyll repository and work on an implementation when possible:

# Gemfile

gem "jekyll", github: "jekyll/jekyll"
...

@mgerzabek
Copy link
Author

@parkr + @ashmaroli rolled back to the minimal required changes to this PR to work with the new additions for Jekyll.

Also sorry for the noise… I enhanced my local copy of this PR to circumvene the downloading of remote theme when it’s present at my local machine and didn’t realize this morning that I have to redo these changes. Everything should be fine now.

Copy link

@parkr parkr left a comment

Choose a reason for hiding this comment

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

I'd love to see a test added for this to ensure that a remote theme _data files are accurately read. 😄

@mgerzabek
Copy link
Author

Mhm… okay, not sure if I‘m able to deliver this… I created a new PR for the Primer theme. It adds a data file greetings.yml with one single variable.

As soon as this PR get’s merged into the theme I could then continue to see how a test would be implented within this repo.

@ashmaroli
Copy link
Contributor

As soon as this PR get’s merged into the theme

@mgerzabek I don't see why you have to wait for a downstream merge..
Your own fork should suffice for low-key testing..:

# _config.yml
remote_theme: mgerzabek/primer@add_data_folder_for_remote_theme

@bedroesb
Copy link

I this actually still needed with Jekyll 4.3.2? With me it already works since the remote theme download and extracts the whole theme repo in temp, and the theme source is set to Theme source: /tmp/jekyll-remote-theme-20230831-18672-p0x48d, it already is reading out the _data files, since it is a 4.3 feature.

@ashmaroli
Copy link
Contributor

I this actually still needed with Jekyll 4.3.2?

Perhaps for cross-compatibility with older versions of Jekyll? This plugin is whitelisted for use with github-pages. That means even users on Jekyll 3.9+ < 4.0 can use the latest version of a theme hosted on GitHub.

@silvinor
Copy link

silvinor commented Oct 6, 2024

Hi there MG, ... I came across your PR because I've been facing a similar problem. In my case, I developed a theme that had a custom 404.html page and also a _data\meta.yml file to encode the default URL of the bootstrap.css file. I allowed the theme user to override the URL in case they wanted to use a custom css on that, but also provide a fallback if they didn't care (and I didn't want to make that a front matter in an _include).

I suppose my want is a mix of this PR and somewhat the description covered in issue #96.

I did look at your code though and tried to see if this was a good starting point - but I have some concerns.

  1. (Not related to code, but...) You've included all your IDEA IDE settings files in your PR (your commit 1ea3611 ).  i.e. files in the .idea and .project folders. This makes it not mergeable. (A merge would mean including these files into the root codebase, and this would not be necessary, as most don't use Idea.)

  2. Your solution is lean and mean... @data_path ||= File.join(@root, "_data")  ... bang!  But ...

  • ||= operand sets if null.  But what if there is already a data_path on the site? Does the theme _data get neglected ... even if the theme has one specific data file, and the site has other data files not present in the theme.  My thinking is that a merger would be better for this need.

  • And, speaking of mergers, should we replace the whole file (of the same name on each side) - making moot any need to merge at all - or do we parse the data from each end, and then merge the data items not in site.data, with those in theme.data ?

Here's wishing BB takes am interest in this ... and creates an answer that I'm not capable of :)

@ashmaroli
Copy link
Contributor

Hello @vinorodrigues Jekyll 4.3 has this feature built-in already. So, feel free to try this feature out with latest version of Jekyll and any version of the remote-theme plugin and provide feedback.

@silvinor
Copy link

silvinor commented Oct 6, 2024

Jekyll 4.3 has this feature built-in already

¿? Which one? Remote theme file inclusion, like 404.html or remote theme _data merger. If so, can you provide a link to the docs? --- because afaik native remote_theme in v4 does neither.
( ty @ashmaroli )

Also ... we're speaking mostly of usage on gh-pages ... I thought that was still on 3.10.0. (Or so they say.)

Aside: ... if only commenting on GH counted towards Stackoverflow xp, hey?

@silvinor silvinor mentioned this pull request Oct 8, 2024
Closed
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.

9 participants