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

Fix support for displayData in uri cell #787

Closed
wants to merge 2 commits into from

Conversation

lukasmasuch
Copy link
Collaborator

@lukasmasuch lukasmasuch commented Oct 4, 2023

The URI cell allows configuring display data, but the behavior differs from other cell types. The display data is only applied to the overlay editor, but not drawn to the canvas. This PR fixes this by preferring displayData in case it is configured.

@jassmith
Copy link
Contributor

jassmith commented Oct 4, 2023

Im a bit wishy/washy on this one, to fix this correctly I think we need to make this draw as a url (underline, blue font?) if we are going to preview with the display data. It also leads to an impression that it should be single clickable.

@jassmith
Copy link
Contributor

jassmith commented Oct 4, 2023

That raises a new even more fun issue, canvas doesn't really properly support underline text

@lukasmasuch
Copy link
Collaborator Author

lukasmasuch commented Oct 4, 2023

@jassmith Oh indeed, I see the issue 😬 I actually have that on my list to investigate if we can improve the URI cell by adding an underline and using the configured link color in rendering. I have seen a version of this here, which attempts to add underline but makes it configurable via an offset. Maybe there is even a way to automatically determine a good underline offset based on the font + size, but I'm not sure ...

The implementation also adds the capability to click on the link directly. But I think this could be something to provide as an optional feature.

@lukasmasuch
Copy link
Collaborator Author

Hmm, since the implementation is using getMiddleCenterBias, it might already adapt well to different fonts and sizes.

@jassmith
Copy link
Contributor

jassmith commented Oct 4, 2023

I already implemented the underline in the link cell in the cells package. The problem is its a bit slow to make it work well. The line needs to break for descenders, which I implemented, but still, slow.

@lukasmasuch
Copy link
Collaborator Author

lukasmasuch commented Oct 4, 2023

Oh nice, I didn't know about link cell 👍 Is the descenders break what makes it slow? Does it need to have the descenders break? I tried underlining in a couple of apps, and some just display it under the text. For example, this is in Notion:

image

It might be good enough to just have one simple line, in case it's significantly better for performance.

@jassmith
Copy link
Contributor

jassmith commented Oct 4, 2023

It's definitely significantly better but its hard to make reliably with arbitrary fonts

@lukasmasuch
Copy link
Collaborator Author

It's definitely significantly better but its hard to make reliably with arbitrary fonts

Maybe we could determine the descenders line by doing some calculation in a similar way as done in getMiddleCenterBias by using a sample with a descenders characters? E.g., if we compare a sample with and one without a descender character we could figure out the relative difference based on the font.

@jassmith
Copy link
Contributor

jassmith commented Oct 4, 2023

This can kind of be made to work but its not as reliable as that method makes it seem. We can try. Honestly I am okay with the perf hit if we code this very very carefully. This is one of those cases where we want to make a test page where we can microbench it and then just try 10 different things and see what is actually fastest. It's going to be one of those cases where one solution will end up being 10x faster.

@jassmith jassmith changed the base branch from main to 6.0.0 November 30, 2023 04:51
@jassmith
Copy link
Contributor

I've taken this PR as is into 6.0.0, sorry for not merging, I couldn't get github to let me commit the merge commit to your branch so this would merge cleanly :(

@jassmith jassmith closed this Nov 30, 2023
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