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

Add ThemeDataReader for data files in theme-gems #5470

Open
wants to merge 6 commits into
base: master
from

Conversation

@ashmaroli
Copy link
Member

commented Oct 9, 2016

If a theme gem contains a _data folder at its root, its files will be read and added to the site's internal data hash. The theme-gem's data hash will be overridden by data at source if the same 'key-value' pair exists there.

TODO:

  • Add tests
  • Add cucumber feature
  • Add Documentation

Ref: #5434
/cc @jekyll/ecosystem, @envygeeks, @pathawks

@pathawks

This comment has been minimized.

Copy link
Member

commented Oct 9, 2016

For what purpose? Seems like a violation of separation of design from content.

I feel like there must be a better way to do i18n.

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2016

For what purpose?

Its not just for i18n per se.. but having this can lead to creating a more functional theme-gem.. I dont have a solid use case for this as of now.. will be fleshed out it in the referenced ticket (no. 5434)

Seems like a violation of separation of design from content.

No.. not all..
though my original intention was to use it for structural elements like define a navigation system using navigation.yml , etc.. I now see the possibilities for built-in i18n as well..

@benbalter

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2016

@ashmaroli thanks for opening up this PR. I really like the vision for themes that you laid out in #5434.

I dont have a solid use case for this as of now.

Before we move forward, I'd feel a lot more comfortable if we understood why we were building the feature, rather than just building it for a sense of symetry with support for other Jekyll-specific folders, or because it's easy to do so. As was mentioned over in #5350, I share @pathawks concerns that themes should only know about presentation, not content (to ensure theme fungability).

We really, really need to figure out a solid I18N implemntation, but I don't know that _data folder support for themes is it.

@ghost

This comment has been minimized.

Copy link

commented Oct 10, 2016

No.. not all..
though my original intention was to use it for structural elements like define a navigation system using navigation.yml , etc.. I now see the possibilities for built-in i18n as well..

Seems like a good use case... for when you are using <site root>/_data/navigation.yml (which already works), not <theme root>/_data/navigation.yml 😄

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2016

I share Pat Hawks' concerns that themes should only know about presentation, not content (to ensure theme fungibility).

I care about theme fungibility too and this feature does not mess with content in the manner you guys fear. Content, to me, comes from markdown files at source, supported by files in an optional _data folder.

@oncleben31

This comment has been minimized.

Copy link

commented Oct 11, 2016

I feel like there must be a better way to do i18n.

We really, really need to figure out a solid I18N implemntation,

Have you some guideline as I'm trying to add basic i18n features to minima template (in jekyll/minima#60) ?
Currently I feel the best option is to use locale string file in _data folder. But I'm new to Jekyll so if you have better ideas please share it.

@ashmaroli ashmaroli force-pushed the ashmaroli:theme-data branch from 1e29e61 to 07845a0 Oct 11, 2016

ashmaroli added some commits Oct 9, 2016

Add ThemeDataReader for data files in theme-gems
If a theme gem contains a '_data' folder at its root, its files will be
read and added to the site's internal data hash.
The theme-gem's data hash will be overridden by data at source if the same
'key-value' pair exists there.

@ashmaroli ashmaroli force-pushed the ashmaroli:theme-data branch from 07845a0 to 9c11c60 Oct 11, 2016

@dummied

This comment has been minimized.

Copy link

commented Nov 13, 2016

Did #5491 (and ashmaroli/jekyll-data) render this moot?

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2016

@dummied This is still active, provided there's a demand for this to be included in the core

@Swiftrien

This comment has been minimized.

Copy link

commented Nov 18, 2016

I have a theme in which I provide default values for configuration and theme-translation via the _data directory.
If not in the _data directory, how else could I provide default values?

@DirtyF DirtyF added the friction label Nov 18, 2016

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2016

@Swiftrien you may also give the jekyll-data gem a try in the meantime..
It provides the same functionality as this pull, and a li'l bit more.. 😃

@Swiftrien

This comment has been minimized.

Copy link

commented Nov 18, 2016

Thanks ashmaroli

However that would mean that every user of my theme also has to install it. Thus this is not the answer to this issue..

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2016

However that would mean that every user of my theme also has to install it.

@Swiftrien I'm not trying to promote a plugin I authored, but if you add jekyll-data as a runtime_dependency to your theme's gemspec, it will be automatically installed with your theme-gem.
Its entirely your call. I won't mind if you decide otherwise.

But I'd definitely like it if you gave it a try, and provide a feedback.
I could then update this pull based on that.

Either way, Happy Jekylling 😃

@Swiftrien

This comment has been minimized.

Copy link

commented Nov 21, 2016

@ashmaroli
I have nothing against promotion :-)
classic-jekyll-theme :-)

I just added the jekyll-data (0.3.0) gem to a website of mine, removed the _data directory, rebuild the site, and everything is as it should be. Thus jekyll-data seems to work fine. Note that I did not use the "theme configuration" only the "regular data" files.

My bigger issue with external contributions is that it takes away my control. And I have had several experiences where external dependencies costs me huge amounts of time. So in general I try to avoid them where possible. As it is, I think that the jekyll-data gem adds enough value that I will probably include it in the next release. (Mainly because I hope it will be part of Jekyll 3.4 anyhow... ;-))

@benbalter benbalter removed their assignment Feb 7, 2017

@ashmaroli ashmaroli referenced this pull request Jan 8, 2018
1 of 1 task complete

@DirtyF DirtyF removed the friction label Jan 8, 2018

@DirtyF DirtyF requested a review from jekyll/core Jan 8, 2018

@yatil

This comment has been minimized.

Copy link

commented Jan 8, 2018

I support this PR for the reasons outlined in #6684.

The actual practical example for us at W3C/WAI is that we have several independent resources that are then sub-moduled into the big site (W3C/wai-website). Every resource can access information in the data files (minor translations but mostly at this stage WCAG 2.0 Success Criteria and techniques). Moving those source files to the theme gives us the possibility to make the changes once and re-use the data in every resource.

@mattr-

This comment has been minimized.

Copy link
Member

commented Jan 18, 2018

This looks ok to me at first glance. I'd be happy to take a deeper look once this up to date with master and the conflicts are resolved.

@parkr

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

The purpose of themes and gem-based themes especially is to make the look-and-feel of websites portable. Can you help me understand how data files, which I usually see used for content, fall under the category of look-and-feel?

@DirtyF

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

@parkr Data files are often used to store config options, that would allow theme authors to embed those options with their themes instead of asking users to copy a sample _config.yml. Right now gem-based themes can only ship layouts and assets and often that's not all it takes to have a working theme out of the box, e.g. @mmistakes: https://github.com/mmistakes/minimal-mistakes/tree/master/_data

@Balancingrock

This comment has been minimized.

Copy link

commented Feb 5, 2018

In the classic-jekyll-theme I use the data files for:

  • Configuration (as DirtyF commented)
  • Translations (User can provide their own translations for titles and headers elements)
  • Data for site elements.

The last one is indeed content data, but the theme will process this data to build the user's site. That imo is definitely a theme-job, but needs user generated data/content.

@yatil

This comment has been minimized.

Copy link

commented Feb 5, 2018

  • Also example data: Here is an example navigation that you can copy to your data directory and edit.
  • Text descriptions for page elements instead of having them in the template or in the _config.yml
@ellemenno

This comment has been minimized.

Copy link

commented Sep 29, 2018

@parkr my look and feel use case is icons: I'm working on a theme that embeds icons as svg, rather than using an icon font.

I want to provide the theme icon shape paths in the _data folder, and provide an _include macro for the theme and the user to embed icons wherever needed.

By retrieving icon data on demand at build time, I can avoid forcing the inclusion of all icons in the generated page html, which saves significant kb.

@ellemenno

This comment has been minimized.

Copy link

commented Nov 8, 2018

Reviewing the posts above, I see at least 7 requests (across two years!) that could be mapped to a couple broad, powerful look & feel use cases:

  • visual configuration
  • asset libraries
    • re-use of intrinsic theme data across multiple projects - yatil, also #6684
    • embeddable icon data - ellemenno

and until jekyll has an official i18n strategy, there is still the use case of simple localization:

@pathawks, @benbalter, @parkr— is there now anything preventing this PR from moving forward?

@Ryuno-Ki

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

@ellemenno As @mattr- wrote in #5470 (comment)

Failing builds + conflicts in the first place :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.