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

feat: add full support for fuzzy linking #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robertkowalski
Copy link
Contributor

this PR adds full support for fuzzy links. to keep backward
compat, the old linkify is still kept.

@robertkowalski
Copy link
Contributor Author

robertkowalski commented Mar 28, 2022

@captainsafia the only difference why i kept the old linkify method for backwards compat is this behaviour:

test("doesn't linkify partial matches", () => {
const el = shallow(
React.createElement(
Ansi,
{ linkify: true },
"cant click this link: 'http://www.google.com'"
)
);
expect(el).not.toBeNull();
expect(el.text()).toBe("cant click this link: 'http://www.google.com'");
expect(el.html()).toBe(
"<code><span>cant click this link: &#x27;http://www.google.com&#x27;</span></code>"
);
});

this becomes a link with the new linkify-it method, even when fuzzylinking is turned off in the lib via options

do you think we should keep this behaviour, or can i introduce a breaking change?

@captainsafia
Copy link
Member

@robertkowalski Thanks for getting this PR out and apologies for being a little slow on the review. I tread a little careful with this link stuff because we have had a few issues in the past with it so it requires some extra caution. 😄

src/index.ts Outdated
if (bundle.bg) {
style.backgroundColor = `rgb(${bundle.bg})`;
}
if (bundle.fg) {
style.color = `rgb(${bundle.fg})`;
}

switch (bundle.decoration) {
Copy link
Member

Choose a reason for hiding this comment

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

Might want to rebase now that the other PR has been merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rebased! :)

src/index.ts Outdated
@@ -85,6 +105,7 @@ function createStyle(bundle: AnserJsonEntry): Colors {

function convertBundleIntoReact(
linkify: boolean,
Copy link
Member

Choose a reason for hiding this comment

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

At the risk of avoiding too many magic booleans in this API, should we make linkify and enum that takes different values for the new vs. old behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea.

i wasn't able to get the typescript type "enum" working with the code without breaking backwards compat, so i extended the type for linkify

this PR adds full support for fuzzy links. to keep backward
compat, the old linkify is still kept.
@robertkowalski
Copy link
Contributor Author

robertkowalski commented Apr 13, 2022

@captainsafia using typescript enums did not work for me without breaking backwards compat... maybe i missed something, but passing the option "fuzzy" did not work for

enum E {
  true,
  false,
  "fuzzy"
}

via:

React.createElement(
  Ansi,
  { linkify: "fuzzy" },
  "this is a fuzzy link: example.com"
)

where the intention is that users from older versions still can use true/false


don't worry on the delay btw

PS: it would be great if you would publish a new version after the merge, i think automatic publishes are broken on master branch currently

@robertkowalski
Copy link
Contributor Author

friendly ping @captainsafia :)

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