[WIP] Implement links (finally only links :D ) #143
Conversation
DominikBucher12
force-pushed
the
feature/final_link
branch
from
2ab9384
to
ee4919b
Compare
| // all undocumented attributes from itself and keeps only the documented ones | ||
| // (In customLinks case - `.link` and `.foregroundColor` | ||
| // This is probably hack because it intervenes with other links (mentions implementation in future) | ||
| if let linkString = textView.richTextAttributes[.link] as? String, |
There was a problem hiding this comment.
Use different method with sync.
|
@DominikBucher12 Is this ready for review? |
| @@ -77,6 +77,8 @@ public enum RichTextAction: Identifiable, Equatable, RichTextLabelValue { | |||
|
|
|||
| /// Undo the latest change. | |||
| case undoLatestChange | |||
|
|
|||
| case link(url: URL?) | |||
There was a problem hiding this comment.
I'd prefer if we keep the alphabetical order for this enum and all switches over it.
| @@ -158,6 +161,7 @@ public extension RichTextAction { | |||
| case .stepSuperscript(let steps): .actionStepSuperscript(steps) | |||
| case .toggleStyle(let style): style.titleKey | |||
| case .undoLatestChange: .actionUndoLatestChange | |||
| case .link: .actionCopy // TODO: Link | |||
There was a problem hiding this comment.
Add this image before merging, just add a link image that uses the "link" symbol.
| } | ||
|
|
||
| public struct CustomLinkAttributes { | ||
| init(link: String, color: ColorRepresentable) { |
There was a problem hiding this comment.
Can we call this RichTextLinkAttribute?
Also, what are the properties? Is link the url? If so, by renaming the type as suggested, we could use explicit naming and call this url or urlString.
| /// This method takes the range, checks if there are any attributes for link and removes them. | ||
| /// After that, sets default typing attributes so we can continue writing as is. | ||
| /// Note if nothing is selected, we just set typing attributes so we can continue writing in normal pace | ||
| func unsetLinkFromCurrentRichTextStyle() { |
There was a problem hiding this comment.
We no longer use the current term for the component, so I think renaming this to removeLink would be enough.
| } | ||
|
|
||
| /// Set the current value of a certain rich text style. | ||
| func setCurrentRichTextLink(_ link: URL?) { |
There was a problem hiding this comment.
And rename this to addLink(to url: URL?)?
| private var subscription: NSObjectProtocol! | ||
|
|
||
| #if macOS | ||
| func textViewDidChange(_ notification: Notification) { |
There was a problem hiding this comment.
Plus, this is the exact same code twice.
| @@ -53,6 +53,7 @@ public extension Image { | |||
| static let richTextStyleItalic = symbol("italic") | |||
| static let richTextStyleStrikethrough = symbol("strikethrough") | |||
| static let richTextStyleUnderline = symbol("underline") | |||
| static let richTextKindLink = symbol("link") | |||
There was a problem hiding this comment.
richTextLink is enough :) Place this in alpha order and use it as instructed above 👍
| @@ -183,6 +183,20 @@ open class RichTextView: UITextView, RichTextViewComponent { | |||
|
|
|||
| // MARK: - Open Functionality | |||
|
|
|||
| open func renderLinks(in characterRange: NSRange, at origin: CGPoint) { | |||
There was a problem hiding this comment.
These two are almost identical, but should be able to be identical as well, right? If so, can we instead extend the component protocol?
| @@ -0,0 +1,53 @@ | |||
| // PresentationContainer.swift | |||
There was a problem hiding this comment.
Regarding placement of the file, this is not a style so it should be elsewhere. Perhaps add a Presentation root folder?
| @@ -41,6 +41,9 @@ public extension RichTextStyle { | |||
| self.styles = styles | |||
| } | |||
|
|
|||
| @State private var urlString = "" | |||
There was a problem hiding this comment.
The link button should not be part of the styles group.
|
@DominikBucher12 I have reviewed the PR in its current state. It will be amazing to get link support, but this PR needs a bit more work before we can merge it. |
danielsaidi
force-pushed
the
main
branch
8 times, most recently
from
fbfbe5d
to
1c86ddc
Compare
danielsaidi
force-pushed
the
main
branch
9 times, most recently
from
be84103
to
80a8ce5
Compare
danielsaidi
force-pushed
the
main
branch
8 times, most recently
from
07211c1
to
d6d8f12
Compare
|
What's the current status on having |
We will have a nice call with @danielsaidi on how to polish this so we can merge this ASAP :) dont mind the formatting, this is still WIP