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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make text available to all span tree nodes #55

Merged
merged 2 commits into from
May 5, 2022

Conversation

angeloashmore
Copy link
Member

@angeloashmore angeloashmore commented May 3, 2022

Types of changes

  • Chore (a non-breaking change which is related to package maintenance)
  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

This PR makes the text property available to all span tree nodes. This is helpful when a span's text is needed to compute its serialized output. Without this PR, spans do not have any contextual information, only their start, end, own node data, and children.

See the following examples to see the difference in children nodes before and after this PR. Note that the text property is absent in the strong node.

Before (key properties omitted):

[
  {
    "children": [],
    "node": {
      "spans": [],
      "text": "Hello ",
      "type": "span"
    },
    "text": "Hello ",
    "type": "span"
  },
  {
    "node": {
      "end": 10,
      "start": 5,
      "type": "strong"
    },
    "type": "strong"
  },
  {
    "children": [],
    "node": {
      "spans": [],
      "text": " World",
      "type": "span"
    },
    "text": " World",
    "type": "span"
  }
]

After (key properties omitted):

[
  {
    "children": [],
    "node": {
      "spans": [],
      "text": "Hello ",
      "type": "span"
    },
    "text": "Hello ",
    "type": "span"
  },
  {
    "node": {
      "end": 10,
      "start": 5,
      "text": "bold",
      "type": "strong"
    },
    "text": "bold",
    "type": "strong"
  },
  {
    "children": [],
    "node": {
      "spans": [],
      "text": " World",
      "type": "span"
    },
    "text": " World",
    "type": "span"
  }
]

Checklist:

  • My change requires an update to the official documentation.
  • All TSDoc comments are up-to-date and new ones have been added where necessary.
  • All new and existing tests are passing.

馃悋

@codecov-commenter
Copy link

Codecov Report

Merging #55 (50d33a7) into master (974bbed) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #55   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          113       115    +2     
  Branches        23        23           
=========================================
+ Hits           113       115    +2     
Impacted Files Coverage 螖
src/asTree.ts 100.00% <100.00%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 974bbed...50d33a7. Read the comment docs.

@angeloashmore angeloashmore requested a review from lihbr May 3, 2022 04:25
@angeloashmore angeloashmore changed the title feat: make text available to all tree nodes feat: make text available to all span tree nodes May 3, 2022
Copy link
Member

@lihbr lihbr left a comment

Choose a reason for hiding this comment

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

Code LGTM

Can you be more specific though regarding real-world use cases of that addition? I'm not sure I really get it/where this is preferred to text from the serializer 馃

Are there considerations regarding node typing also?

(@prismicio/types for reference: https://github.com/prismicio/prismic-types/blob/master/src/fields.ts#L115-L146, those describe the API node type and shouldn't be changed)

@angeloashmore
Copy link
Member Author

Can you be more specific though regarding real-world use cases of that addition? I'm not sure I really get it/where this is preferred to text from the serializer 馃

Yep, I probably should have included that in the original message. 馃槅

This lets an HTML serializer use the span's text contents in its output. For example, one could use it to assign an ID to a piece of text.

Input (assume the bolded text is a label with the name "anchor"):

This text contains a label[#anchor] with an ID.

Output:

<p>This text contains <span id="anchor">a label</span> with an ID.

This could also be used for links.

Output:

<p>This text contains <PrismicLink href={theResolvedLink} anchor={anchor}>a link</PrismicLink> with an ID.</p>

It could also be used to add automatic slugs to spans, just like we can do with headings. Headings can do that today because they have access to the block's text. Spans cannot do it today because they do not have access to its parent's text.

<p>This text contains <span id="a-label">a label</span> with an ID.</p>

Or titles:

This text contains text with a title[title:Hello reader] with an ID.

<p>This text contains <span title="Hello reader">a label</span> with an ID.</p>

While the syntax shown here is not necessarily the most optimal way of performing these tasks, they sufficiently demonstrate that a span could manipulate its text if it has access to it.

(Credit to @chamois-d-or for the idea and asking if this was possible)

Are there considerations regarding node typing also?

No, there shouldn't be any. The nodes are already typed with { text?: string }, but only block-level nodes contained a value. The type is still correct since an image node, for example, would still not contain a text property.

@lihbr
Copy link
Member

lihbr commented May 5, 2022

OK, got it thank you!

So this type was wrong essentially as text would have always been undefined: https://github.com/prismicio/prismic-richtext/blob/master/src/types.ts#L125

I'll let you merge/release 鈽猴笍

@angeloashmore
Copy link
Member Author

So this type was wrong essentially as text would have always been undefined: https://github.com/prismicio/prismic-richtext/blob/master/src/types.ts#L125

Basically, yes. The tree node that was provided to any span serializer function (label, strong, em, etc.) would never receive the text argument/property. Technically { text?: string } is correct, but it would have been more accurate to provide { text: never } to span functions.

With this PR, the types are more correct.

Thanks for the review! 馃檪

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.

None yet

3 participants