-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
I like it; I'll try it out later this afternoon with my releases PR in #19 ! |
@patricoferris I added a preview for the books. FYI there were a lot of missing/wrong cover images, not sure what went wrong on the import, it should be fixed now! |
@tmattio thanks! Yeah my bad, now that we have a place for images we should add a lint step to optionally check that the referenced image does in fact exist... would have come in handy :)) |
@patricoferris I just added the tutorial previews, so there is now a preview for all of the data in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmattio this is such a nice piece of work, thank you it should make the back and worth of data-editing/making and prototyping designs using that data a much better experience. I left some minor comments but nothing major (or anything I feel is blocking this). I trust your judgement to merge when you are happy :))
| `Advanced | ||
] | ||
|
||
type metadata = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never seen this type t = <some-type-alias> = <type-def>
before, what is the purpose? It has been used for the other entries (e.g. Success_story
), should this be
type metadata = Tutorial.t = ...
Could we not just pull in the information from the library in ood-cli
which has the types or is it so you can rename the type to metadata
and produce derivers for this new type name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I use it to generate the YAML converters functions with the ppx.
The problem is, the yaml ppx deriver does not seem to support the [@key ""]
(or is it [@name ""]
attribute for polymorphic variants, so I need to use a string instead and convert to the polymorphic variant manually. That's why I didn't add the = Tutorial.t
in this case, because the types are not actually equal.
To share a few thoughts related to this question: I was wondering why the types in Ood
don't include the body
field?
Also, we could make Ood
depend on ppx_deriving_yaml
, I think the ppx syntax will just be ignored by ReScript, right? That would make the types in Ood
much more reusable in Ood_preview
.
Re: "Could we not just pull in the information from the library in ood-cli which has the types"
I didn't look much into ood-cli
. I'm not sure which types we would pull in, because we use already defined types in Ood
(with the exception of Tutorial.metadata
here). What would be the benefit of depending on ood-cli
? One immediate drawback is that ood-cli
depends on a lot of irrelevant things for the ood-preview
, e.g. the netlify stuffs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the variant thing is annoying, I have an issue related to it patricoferris/ppx_deriving_yaml#23
I don't think there is any particular reason they don't include the body, it was just for clearer distinction between metadata and markdown body. Maybe it is possible to share some of this across the collections similar to what I did in sesame as only the metadata changes between them.
Also, we could make Ood depend on ppx_deriving_yaml, I think the ppx syntax will just be ignored by ReScript, right? That would make the types in Ood much more reusable in Ood_preview.
I don't know enough about ReScript but I think the problem is ppx_deriving_yaml
needs Yaml
which needs C-bindings and at that point things break. If we could somehow fix this and have single unified set of types I would be happy.
I didn't look much into ood-cli. I'm not sure which types we would pull in, because we use already defined types in Ood (with the exception of Tutorial.metadata here)
What about https://github.com/ocaml/ood/blob/main/src/ood-cli/data.ml ? I just realised the last PR completely removed lib_ood
which is what I was really referring to here. But I think you are right about dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know enough about ReScript but I think the problem is ppx_deriving_yaml needs Yaml which needs C-bindings and at that point things break.
I was thinking that the deriver would simply be ignored and no code depending on Yaml would be generated for ReScript. In OCaml, this would generate that the other libs depend on.
But now that I'm writing this, it seems like a very obscure and confusing setup, so probably not a good idea.
I don't think there is any particular reason they don't include the body, it was just for clearer distinction between metadata and markdown body.
Could we add types for metadata
and t
in this case? where t
would be
type t = {
meta: metadata;
body: string
}
just realised the last PR completely removed lib_ood which is what I was really referring to here.
Oops, I should have mentioned it in the PR more explicitly. I did this to simplify the setup a bit, but if you believe there was a use for it being provided as a lib, I'll revert this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're ok with it, I'll merge the PR without reworking the types. It seems like an important conversation, so I'd prefer to have it in its own PR, that I can open once this is merged.
open Import | ||
|
||
let extract_metadata_body s = | ||
match Str.split (Str.regexp "---") s with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use jekyll-format for this but seems this is working haha :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice! I didn't know about jekyll-format. The function is really hacky, so I'm happy to replace it :)
jekyll-format comes with a lot of new dependencies for a single function though, so maybe we can simply copy the relevant bits? (https://github.com/avsm/jekyll-format/blob/master/src/jekyll_format.ml#L17)
|
||
let map_files f dir = read_from_dir dir |> List.map f | ||
|
||
let slugify value = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering how best to do relational entries in the metadata, the way it happened before through NetlifyCMS was either custom or you could just specify a field from one collection to another (e.g. the conference could reference a video by adding the video title field), but "slugifying" them might be a better option, the question is how to share this with the front-end, any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The slugs are only used for URLs at the moment, but if we're going to support relations in the data (which I would refrain from doing as much as possible because of the extra complexity), I think we'll need a better heuristic (e.g. the file names seem better for the data located in folders)
I use this function in id_of_t
or get_by_id
, which is confusing, I should replace ID by slug, which is more accurate.
while true; do | ||
make build | ||
$cmd & | ||
fswatch -r -1 $source_dirs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have fswatch
installed so I got stuck in a while-loop, could we add a quick which fswatch
or something similar and print a message if it's not installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea :)
I addressed the review comments, except for the type sharing discussion, I'll open an issue/PR to discuss what we want to do. |
This PR adds a small web app to preview the content offered by
ood
.While preparing to migrate the content from the old website to the new one, I thought this would offer a nice development workflow with the following benefits:
v3
and adapt the syntaxThis is just me setting up my dev workflow for the coming weeks, I'm opening in case others find it useful as well, but don't feel any need to merge if that's not the case 🙂
This is based on top of #20 to implement a preview of the success stories.
To run the preview app, you can run
make deps
to install the dependencies, thenmake preview
to run the app in watch mode + live reload, ordune exec preview/bin/server.exe run
to run the server binary only.