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

Do not process italic syntax on links. #3

Merged
merged 7 commits into from
Aug 21, 2023

Conversation

kublaios
Copy link
Collaborator

Steps to reproduce

  • send a markdown link like [schedule](https://care.kaiahealth.com/appointments/embed_appt?dietitian_id=1167309) through the healthie chat and click on it

Actual Result

  • the link does not contain underscores and therefore does not open correctly in the browser

Expected result

  • the link works normally

@@ -9,7 +9,7 @@ import Foundation

open class MarkdownItalic: MarkdownCommonElement {

fileprivate static let regex = "(.?|^)(\\*|_)(?=\\S)(.+?)(?<![\\*_\\s])(\\2)"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This regex was implemented in bmoliveira@761a30e to "fix" when the italic syntax is combined with bold and/or other characters, but it was capturing the preceding character (e.g., when capturing _italic_, it would capture _italic_ with the space before. I reverted it as it doesn't make much sense for our use case, and it still works with bold text.

@kublaios kublaios requested review from a team, russellporter, alex-deutsch, ahmad-atef and AymenLetaief and removed request for a team August 17, 2023 17:41
Copy link

@russellporter russellporter left a comment

Choose a reason for hiding this comment

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

nit: it could be nice to update the tests to cover this case

Comment on lines 61 to 64
let attributedSubstring = attributedString.attributedSubstring(from: range)
var effectiveRange = NSRange(location: 0, length: 0)
let atributes = attributedSubstring.attributes(at: 0, effectiveRange: &effectiveRange)
return atributes.keys.contains(attribute)

Choose a reason for hiding this comment

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

nit: (assuming we don't need to check the whole range) I think it can be simplified to:

attributedString.attribute(attribute, at: range.position, effectiveRange: nil) != nil

private func attributedString(
_ attributedString: NSAttributedString,
hasAttribute attribute: NSAttributedString.Key,
at range: NSRange

Choose a reason for hiding this comment

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

It looks to me that this function only checks that there is an attribute at the first position in the range, right?

If that's intended maybe we can change the param to at location: Int to make it clearer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, it turned out that we don't need this check at all - just updating the regex was sufficient, so I reverted this part. Thanks for the nudge!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the demo template to showcase more cases (link with underscores and bold+italic vs italic+bold)

@@ -159,8 +159,8 @@ open class MarkdownParser {
(.bold, bold),
(.italic, italic),
(.strikethrough, strikethrough),
(.link, link),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When the link was being parsed before, it would join the text coming after the link, and the autolink would incorrectly make the following text a part of the link.

Copy link

@russellporter russellporter left a comment

Choose a reason for hiding this comment

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

short n'sweet 😻

@kublaios kublaios merged commit 8e0ed79 into master Aug 21, 2023
1 check failed
@kublaios kublaios deleted the dev/kubilay/KAIA-20477-fix-underscores-in-URLs branch August 21, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants