Type link: Fixed ending punctuation to recognise links. Closes #14497. #16264
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Made it so that when typing, including copy and paste, typing ending punctuations will have the link be recognised as a link. Added only for ending so parameters using ? are not affected.
WalkthroughThe update enhances the Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Comment on lines
338
to
346
| function isSingleSpaceOrPunctuationAtTheEnd( text: string ): boolean { | ||
| return ( text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ' ' || | ||
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === '.' || | ||
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === '!' || | ||
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ':' || | ||
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ',' || | ||
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ';' || | ||
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === '?' | ||
| ) && text[ text.length - 2 ] !== ' '; |
There was a problem hiding this comment.
Refactor the function for improved readability and performance.
- function isSingleSpaceOrPunctuationAtTheEnd( text: string ): boolean {
- return ( text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ' ' ||
- text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === '.' ||
- text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === '!' ||
- text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ':' ||
- text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ',' ||
- text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ';' ||
- text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === '?'
- ) && text[ text.length - 2 ] !== ' ';
- }
+ function isSingleSpaceOrPunctuationAtTheEnd( text: string ): boolean {
+ const lastChar = text[ text.length - 1 ];
+ const validPunctuation = ' .!:,;?'.includes(lastChar);
+ return text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && validPunctuation && text[ text.length - 2 ] !== ' ';
+ }This refactor simplifies the function by using a single string to check for valid punctuation marks, improving both readability and performance by reducing the number of conditions checked.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| function isSingleSpaceOrPunctuationAtTheEnd( text: string ): boolean { | |
| return ( text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ' ' || | |
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === '.' || | |
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === '!' || | |
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ':' || | |
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ',' || | |
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === ';' || | |
| text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && text[ text.length - 1 ] === '?' | |
| ) && text[ text.length - 2 ] !== ' '; | |
| function isSingleSpaceOrPunctuationAtTheEnd( text: string ): boolean { | |
| const lastChar = text[ text.length - 1 ]; | |
| const validPunctuation = ' .!:,;?'.includes(lastChar); | |
| return text.length > MIN_LINK_LENGTH_WITH_SPACE_AT_END && validPunctuation && text[ text.length - 2 ] !== ' '; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
Actionable comments outside the diff hunks (3)
packages/ckeditor5-link/tests/manual/autolink.md (3)
Line range hint
1-1: Correct the spelling mistake in "AutoLink feature".- AutoLink feature + AutoLink Feature
Line range hint
1-9: Ensure all list items start with a capital letter.- staring with `http://`. - staring with `https://`. - staring without a protocol (www.cksource.com). + Starting with `http://`. + Starting with `https://`. + Starting without a protocol (www.cksource.com).
Line range hint
32-32: Use underscores for emphasis instead of asterisks to maintain consistency.- Check if *only* created link was removed: + Check if _only_ created link was removed:
| 1. A Period (`.`) | ||
| 2. An exclamation mark (`!`) | ||
| 3. A Colon (`:`) | ||
| 4. A comma (`,`) |
There was a problem hiding this comment.
Add a space after the comma for better readability.
- 4. A comma (`,`)
+ 4. A comma (`, `)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| 4. A comma (`,`) | |
| 4. A comma (`, `) |
| @@ -10,6 +10,15 @@ | |||
| 2. Type space after a URL. | |||
| 3. Check if text typed before space get converted to link. | |||
|
|
|||
| #### _Special characters_ | |||
There was a problem hiding this comment.
Add a blank line above the heading for better markdown formatting.
+
#### _Special characters_Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| #### _Special characters_ | |
| #### _Special characters_ |
Comment on lines
+15
to
+21
| 1. A Period (`.`) | ||
| 2. An exclamation mark (`!`) | ||
| 3. A Colon (`:`) | ||
| 4. A comma (`,`) | ||
| 5. A Semicolon (`;`) | ||
| 6. A Question mark (`?`) | ||
|
|
There was a problem hiding this comment.
Surround the list with blank lines for proper markdown formatting.
+
1. A Period (`.`)
2. An exclamation mark (`!`)
3. A Colon (`:`)
4. A comma (`,`)
5. A Semicolon (`;`)
6. A Question mark (`?`)
+ Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| 1. A Period (`.`) | |
| 2. An exclamation mark (`!`) | |
| 3. A Colon (`:`) | |
| 4. A comma (`,`) | |
| 5. A Semicolon (`;`) | |
| 6. A Question mark (`?`) | |
| 1. A Period (`.`) | |
| 2. An exclamation mark (`!`) | |
| 3. A Colon (`:`) | |
| 4. A comma (`,`) | |
| 5. A Semicolon (`;`) | |
| 6. A Question mark (`?`) | |
Comment on lines
252
to
306
| it( 'adds linkHref attribute to a text link after space( )', () => { | ||
| simulateTyping( 'https://www.cksource.com ' ); | ||
|
|
||
| expect( getData( model ) ).to.equal( | ||
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text> []</paragraph>' | ||
| ); | ||
| } ); | ||
|
|
||
| it( 'adds linkHref attribute to a text link after period(.)', () => { | ||
| simulateTyping( 'https://www.cksource.com.' ); | ||
|
|
||
| expect( getData( model ) ).to.equal( | ||
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>.[]</paragraph>' | ||
| ); | ||
| } ); | ||
|
|
||
| it( 'adds linkHref attribute to a text link after exclamation mark(!)', () => { | ||
| simulateTyping( 'https://www.cksource.com.' ); | ||
|
|
||
| expect( getData( model ) ).to.equal( | ||
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>![]</paragraph>' | ||
| ); | ||
| } ); | ||
|
|
||
| it( 'adds linkHref attribute to a text link after colon(:)', () => { | ||
| simulateTyping( 'https://www.cksource.com.' ); | ||
|
|
||
| expect( getData( model ) ).to.equal( | ||
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>:[]</paragraph>' | ||
| ); | ||
| } ); | ||
|
|
||
| it( 'adds linkHref attribute to a text link after comma(,)', () => { | ||
| simulateTyping( 'https://www.cksource.com.' ); | ||
|
|
||
| expect( getData( model ) ).to.equal( | ||
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>,[]</paragraph>' | ||
| ); | ||
| } ); | ||
|
|
||
| it( 'adds linkHref attribute to a text link after semi-colon(;)', () => { | ||
| simulateTyping( 'https://www.cksource.com.' ); | ||
|
|
||
| expect( getData( model ) ).to.equal( | ||
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>;[]</paragraph>' | ||
| ); | ||
| } ); | ||
|
|
||
| it( 'adds linkHref attribute to a text link after question mark(?)', () => { | ||
| simulateTyping( 'https://www.cksource.com.' ); | ||
|
|
||
| expect( getData( model ) ).to.equal( | ||
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>?[]</paragraph>' | ||
| ); | ||
| } ); |
There was a problem hiding this comment.
Ensure the correct punctuation mark is used in each test case.
- simulateTyping( 'https://www.cksource.com.' );
+ simulateTyping( 'https://www.cksource.com' );Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| it( 'adds linkHref attribute to a text link after space( )', () => { | |
| simulateTyping( 'https://www.cksource.com ' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text> []</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after period(.)', () => { | |
| simulateTyping( 'https://www.cksource.com.' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>.[]</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after exclamation mark(!)', () => { | |
| simulateTyping( 'https://www.cksource.com.' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>![]</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after colon(:)', () => { | |
| simulateTyping( 'https://www.cksource.com.' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>:[]</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after comma(,)', () => { | |
| simulateTyping( 'https://www.cksource.com.' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>,[]</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after semi-colon(;)', () => { | |
| simulateTyping( 'https://www.cksource.com.' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>;[]</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after question mark(?)', () => { | |
| simulateTyping( 'https://www.cksource.com.' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>?[]</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after space( )', () => { | |
| simulateTyping( 'https://www.cksource.com ' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text> []</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after period(.)', () => { | |
| simulateTyping( 'https://www.cksource.com.' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>.[]</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after exclamation mark(!)', () => { | |
| simulateTyping( 'https://www.cksource.com!' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>![]</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after colon(:)', () => { | |
| simulateTyping( 'https://www.cksource.com:' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>:[]</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after comma(,)', () => { | |
| simulateTyping( 'https://www.cksource.com,' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>,[]</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after semi-colon(;)', () => { | |
| simulateTyping( 'https://www.cksource.com;' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>;[]</paragraph>' | |
| ); | |
| } ); | |
| it( 'adds linkHref attribute to a text link after question mark(?)', () => { | |
| simulateTyping( 'https://www.cksource.com?' ); | |
| expect( getData( model ) ).to.equal( | |
| '<paragraph><$text linkHref="https://www.cksource.com">https://www.cksource.com</$text>?[]</paragraph>' | |
| ); | |
| } ); |
|
Hi! Thank you for PR, I checked it and spotted minor issues related to tests and detecting of the punctuation. I created a new one: #17680, and it's merged to master. I'm closing this PR, as the issue should be solved right now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Made it so that when typing, including copy and paste, typing ending punctuations will have the link be recognised as a link. Added only for ending so parameters using ? are not affected.
Type: Message. Closes #14497
Summary by CodeRabbit