Skip to content

Rewrite inline ranges as a tree#5

Merged
buob merged 1 commit intomasterfrom
jb/tree
Mar 4, 2019
Merged

Rewrite inline ranges as a tree#5
buob merged 1 commit intomasterfrom
jb/tree

Conversation

@buob
Copy link

@buob buob commented Mar 1, 2019

I did say "today" 😅

Admittedly I haven't worked with tree structures enough to know all of the paradigms, so there might be a way to write the build_tree function and friends that is a bit more idiomatic, fewer lines of code, and/or more logically recursive, I'd love to refactor to that end now that I finally have something working. 🎉

This made us able to do some unique things, like transforming content
in our custom entities, that we were unable to do with our simpler
approach.
@buob buob requested a review from crossman March 1, 2019 23:21
|> divvy_style_ranges(block["entityRanges"])
|> group_style_ranges()
|> Kernel.++(block["entityRanges"])
|> sort_by_offset_and_length_then_styles_first()
Copy link

Choose a reason for hiding this comment

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

Obj-C naming is leaking

Copy link
Author

Choose a reason for hiding this comment

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

I'm unapologetic 😂

Unless you want me to be apologetic.

Then I'll be apologetically apologetic. 😏 🙇

@crossman
Copy link

crossman commented Mar 4, 2019

Yeah aside from the one comment comment above I'm fine with merging this and tossing a refactor in the icebox. I would like to break this up further in the future so that this actually just builds the tree and then the lib user brings their own processor so that it can be used for rendering to non-html clients 🦄

block["inlineStyleRanges"]
|> divvy_style_ranges(block["entityRanges"])
|> group_style_ranges()
|> Kernel.++(block["entityRanges"])
Copy link

Choose a reason for hiding this comment

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

Alright took a little while to grok that the entityRanges weren't actually in the data until this point... I don't know if I have an immediate action point for this but I think this pipe could be a little cleaner. Probably no harm leaving it for the refactor you mentioned. Maybe a brief comment at the top would help though

@buob buob merged commit c6572fc into master Mar 4, 2019
@buob buob deleted the jb/tree branch March 4, 2019 18:17
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