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

Rich text comments #108

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

tom-sherman
Copy link
Contributor

No description provided.

Copy link

linear bot commented Jul 19, 2024

Copy link

vercel bot commented Jul 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
atproto-browser ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 22, 2024 4:08pm
frontpage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 22, 2024 4:08pm
unravel ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 22, 2024 4:08pm

Comment on lines 3 to 6
// FIXME: Remove this when we're closer to production
typescript: {
ignoreBuildErrors: true,
},
eslint: {
ignoreDuringBuilds: true,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must remember to remove this later...


type CommentContent = string | { content: string; facets: CommentFacet[] };

function $nodeToCommentContent(node: LexicalNode): CommentContent[] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some notes on the ComentContent[] type for later as this will be included in our lexicon:

There are two representations for some text without any facets:

  • "foo"
  • {content: "foo", facets: []}

I'm not sure there are good ways around this without making the type much harder to encode and decode.

We force an array type even though CommentContent | CommentContent[] would be equally valid and not much more difficult to implement. This is to make it easier to decode later.

This reduces the amount of possible empty value representations by one, but there are still a few. These are all equivalent and mean "empty value"

  • empty array: []
  • empty array with string: [""]
  • empty array with obj: [{content: "", facets: []}]
  • empty array with many strings/objs: ["", {content: "", facets: []}]

This actually leads to there being an infinite amount of possible empty values... This is really not ideal because it can't easily be caught by putting char length constraints on the text, as no matter how many nodes you have the length of the concatenated string will always be 0. I think this needs to be fixed somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having multiple ways of doing something in the data model is bad because in general the more ways there are to do something, the more complex it is to enforce constraints, which leads to a higher likelihood of a loophole being possible.

Copy link
Contributor Author

@tom-sherman tom-sherman Jul 28, 2024

Choose a reason for hiding this comment

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

Worth noting that we're kind of constrained by the atproto lexicon spec here, some solutions (such as non-empty types) are not possible right now.

We can abandon this tho, and consider rich-text fields as unknown in the lexicon for now while we wait for a proper richtext type to be added to the spec I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally thought that a flat array would suitable here, but actually nested nodes need to be supported.

Consider a link that has some characters bolded, but not all. In markdown:

[some **link**](example.com)

We don't want to represent this as a flat array because the best we can do with that is:

[some ](example.com)[**link**](example.com)

This is not what the author intended. Adding heuristics to merge sibling links is not possible either because it should be valid to have two links that are the same next to eachother in the text.

The question now becomes: should rich text nodes be able to be nested arbitrarily or are there some rules we should put in place here to prevent arbitrary recursion depths?

Copy link
Contributor Author

@tom-sherman tom-sherman Jul 28, 2024

Choose a reason for hiding this comment

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

The nested structure also introduces another way to represent empty text:

[
  {
    content: [
      {
        content: [...],
        facets: []
      }
    ],
    facets: []
  }
]

@tom-sherman tom-sherman force-pushed the tom/un-90-rich-text-in-comments branch from d2f6b1b to ebd257f Compare November 22, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant