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 a bodyxml->content-tree converter #49

Merged
merged 22 commits into from
Feb 5, 2024
Merged

Add a bodyxml->content-tree converter #49

merged 22 commits into from
Feb 5, 2024

Conversation

chee
Copy link
Member

@chee chee commented Jan 26, 2024

This is only made to deal with very simple articles so far that contain basic html content.

I need to do a little more research to understand what <content type=content and <content type=article are.

It passes the simple-post test but not the kitchen-snippet test.

The transformer API is fine, i guess. you write something like this:

["pull-quote"](pq) {
	let text = find(pq, {name: "pull-quote-text"})
	let source = find(pq, {name: "pull-quote-source"})
	return {
		type: "pullquote",
		text: text ? xastToString(text) : "",
		source: source ? xastToString(source) : "",
		children: null,
	}
}

if you return a children value, it'll recurse into the children of the xmlnode to make the children. if you return a children value, it won't. if you return children: null it won't go to the children but also the children will be deleted from the node.

hope u like it!

@adgad
Copy link
Collaborator

adgad commented Jan 26, 2024

API looks okay to me so far. the ones are gonna be interesting, I reckon there are ones that behave differently depending the attributes (e.g. flourish i think has type Content, and a data-asset-type=\"flourish\"

I guess that can still work though, we can return different things inside the transformer function depending on what the attribute is.

} else if ("children" in xmlnode) {
return {
...ctnode,
// this is a flatmap because of <experimental/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

😭

@chee
Copy link
Member Author

chee commented Jan 26, 2024

API looks okay to me so far. the ones are gonna be interesting, I reckon there are ones that behave differently depending the attributes (e.g. flourish i think has type Content, and a data-asset-type=\"flourish\"

I guess that can still work though, we can return different things inside the transformer function depending on what the attribute is.

i kept making the API more powerful and then stripping it away when none of the nodes i was working with needed the power. but it's possible that the transformer function needs to take a second arg of walk() which'll let them take care of their own children and allow the main walk() loop to know less about the different kinds of nodes

@chee
Copy link
Member Author

chee commented Jan 26, 2024

API looks okay to me so far. the ones are gonna be interesting, I reckon there are ones that behave differently depending the attributes (e.g. flourish i think has type Content, and a data-asset-type=\"flourish\"
I guess that can still work though, we can return different things inside the transformer function depending on what the attribute is.

i kept making the API more powerful and then stripping it away when none of the nodes i was working with needed the power. but it's possible that the transformer function needs to take a second arg of walk() which'll let them take care of their own children and allow the main walk() loop to know less about the different kinds of nodes

another option is to make the code more about mutating the existing tree than generating a new one. that's probably how we'd do this if we weren't javascript programmers. that code would look more like using unist-util-visit, with the return value being used to decide how or if to recurse.

@chee
Copy link
Member Author

chee commented Jan 26, 2024

i should add some more tests. hopefully i can get a hold of some specialist content soon to use as a source of tests

@chee chee marked this pull request as ready for review January 31, 2024 08:47
@chee chee requested review from a team as code owners January 31, 2024 08:47
@chee
Copy link
Member Author

chee commented Jan 31, 2024

for more complicated transforms like the ones in cp we might need to provide the parent node to the transformer function as a second arg

@chee
Copy link
Member Author

chee commented Jan 31, 2024

once this is merged i'll go ahead and enact the filesystem changes described in a comment on eli's PR because we've confirmed we can make that work using a go.mod file in the root of the repo beside the root package.json.

then we can start working towards a world where this repo can publish packages.

Copy link
Collaborator

@adgad adgad left a comment

Choose a reason for hiding this comment

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

Happy to approve this! I guess the proof in the pudding will be once we're able to publish it as a package and @vbachev and co can start using it 🍿

@chee chee merged commit 65a0ad7 into main Feb 5, 2024
2 checks passed
@chee chee deleted the bodyxml-to-content-tree branch February 5, 2024 15:59
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