Excerpt will display properly if there are chinese-japanese-korean characters. #2262
Open
prconcepcion wants to merge 9 commits intodevelopfrom
Open
Excerpt will display properly if there are chinese-japanese-korean characters. #2262prconcepcion wants to merge 9 commits intodevelopfrom
prconcepcion wants to merge 9 commits intodevelopfrom
Conversation
|
Size Change: +1.1 kB (0%) Total Size: 2.75 MB
ℹ️ View Unchanged
|
bfintal
requested changes
May 6, 2022
Comment on lines
+118
to
+119
| // Trim the excerpt differently if there are chinese characters. | ||
| $excerpt = filter_cjk_characters( $excerpt, $trim_to_length ); |
Contributor
There was a problem hiding this comment.
While this would technically work, the logic is off. The CJK trimming shouldn't be called after the if/else statement, because this would mean that the strings will always be trimmed and will be trimmed twice.
Our logic above is as follows (in the if statement):
- If the excerpt is too long, trim it
- If not, just use as-is
The logic in the PR is:
- If the excerpt is too long, trim it
- If not, just use as-is
- After 1 & 2, trim CJK characters
The trimming of CJK should make the logic be:
- If the excerpt has CJK characters, trim it if it's too long,
- If the excerpt doesn't have CJK characters but it's too long, trim it
- If not, just use as-is
bfintal
added
PR: Reviewed w/ Comments
bfintal
reviewed
May 7, 2022
| } elseif ( count( $excerpt ) > $trim_to_length ) { | ||
| $excerpt = implode( ' ', array_slice( $excerpt, 0, $trim_to_length ) ) . '...'; | ||
| } else { | ||
| $excerpt = implode( ' ', $excerpt ); |
Contributor
There was a problem hiding this comment.
Suggested change
| $excerpt = implode( ' ', $excerpt ); | |
| $excerpt = $untrimmed_excerpt; |
bfintal
reviewed
May 7, 2022
| if ( count( $excerpt ) > $trim_to_length ) { | ||
|
|
||
| // Check if there are CJK characters. | ||
| if ( preg_match_all("/\p{Han}+/u", $untrimmed_excerpt, $matches) ) { |
Contributor
There was a problem hiding this comment.
Some recommendations:
- Mind the spaces.
mb_substrisn't always available across all PHP installations, we need to also check for its existence.
Suggested change
| if ( preg_match_all("/\p{Han}+/u", $untrimmed_excerpt, $matches) ) { | |
| if ( function_exists( 'mb_substr' ) && preg_match_all( "/\p{Han}+/u", $untrimmed_excerpt, $matches ) ) { |
bfintal
reviewed
May 7, 2022
|
|
||
| // Check if there are CJK characters. | ||
| if ( preg_match_all("/\p{Han}+/u", $untrimmed_excerpt, $matches) ) { | ||
| if( strlen( $untrimmed_excerpt ) > $trim_to_length ) { |
Contributor
There was a problem hiding this comment.
Suggested change
| if( strlen( $untrimmed_excerpt ) > $trim_to_length ) { | |
| if ( strlen( $untrimmed_excerpt ) > $trim_to_length ) { |
bfintal
reviewed
May 7, 2022
Comment on lines
+207
to
+215
| if ( excerptLength === 1 ) { | ||
| excerptString = postExcerptStackable.substring( 3, 4 ) + '...' | ||
| } else { | ||
| excerptString = postExcerptStackable.substring( 3, excerptLength === '' ? 55 : excerptLength + 3 ) + '...' | ||
| } | ||
| // Outerconditional is always true even though postExcerptStackable.length < 55. | ||
| if ( postExcerptStackable.length < 55 && excerptLength === '' ) { | ||
| excerptString = postExcerptStackable | ||
| } |
Contributor
There was a problem hiding this comment.
-
Why is there a different line for when the
excerptLengthis1, there should be no reason to split this into anif-else -
In this
postExcerptStackable.substring( 3, 4 ), why is the string trimming process started on index3? Shouldn't it start with0? -
What is the last
ifstatement for? Looks unnecessary.
Won't this do? Or is there an edge case where this doesn't work?
Suggested change
| if ( excerptLength === 1 ) { | |
| excerptString = postExcerptStackable.substring( 3, 4 ) + '...' | |
| } else { | |
| excerptString = postExcerptStackable.substring( 3, excerptLength === '' ? 55 : excerptLength + 3 ) + '...' | |
| } | |
| // Outerconditional is always true even though postExcerptStackable.length < 55. | |
| if ( postExcerptStackable.length < 55 && excerptLength === '' ) { | |
| excerptString = postExcerptStackable | |
| } | |
| excerptString = postExcerptStackable.substring( 0, excerptLength || 55 ) + '...' |
bfintal
requested changes
May 7, 2022
Contributor
bfintal
left a comment
bfintal
left a comment
There was a problem hiding this comment.
Please see inline comments for some enhancements
prconcepcion
added
PR: Needs Review
added 2 commits
May 10, 2022 20:22
bfintal
reviewed
May 10, 2022
| if ( function_exists( 'mb_substr' ) && preg_match_all( "/\p{Han}+/u", $untrimmed_excerpt, $matches ) ) { | ||
| if ( strlen( $untrimmed_excerpt ) > $trim_to_length ) { | ||
| // Trim according to string length. | ||
| $excerpt = mb_substr( $untrimmed_excerpt, 3, $trim_to_length ) . '...'; |
Contributor
There was a problem hiding this comment.
Why does the index start with 3, shouldn't it be 0?
Suggested change
| $excerpt = mb_substr( $untrimmed_excerpt, 3, $trim_to_length ) . '...'; | |
| $excerpt = mb_substr( $untrimmed_excerpt, 0, $trim_to_length ) . '...'; |
bfintal
reviewed
May 10, 2022
| const regexCJK = /[\u3040-\u30ff\u3400-\u4dbf\u4e00-\u9fff\uf900-\ufaff\uff66-\uff9f]/ | ||
| if ( postExcerptStackable.match( regexCJK ) ) { | ||
| // Extract the string inbetween the tags and newline. | ||
| const tempExcerptString = postExcerptStackable.match( '<p>(.*)<\/p>\\n' ) |
Contributor
There was a problem hiding this comment.
What scenario is this needed?
Contributor
Author
There was a problem hiding this comment.
The string that I obtained when I would use the postExcerptStackable would contain the <p> which is why I made it get the string between those tags and this is also the reason why I made the function substing(3,excerptLength+3) start at 3 before and then also added a +3. I also saw this in the $untrimmed_excerpt in the php file that is why I also made the index mb_substr( $untrimmed_excerpt, 3, ... start at 3.
Contributor
Author
There was a problem hiding this comment.
The reason why I also made an if (excerptLength === 1 ) ... back then was because it would look like this in the backend and frontend respectively when the excerpt was 1:
bfintal
reviewed
May 10, 2022
Comment on lines
+209
to
+213
| if ( tempExcerptString[ 1 ].length > ( excerptLength || 55 ) ) { | ||
| excerptString = tempExcerptString[ 1 ].substring( 0, excerptLength === '' ? 55 : excerptLength ) + '...' | ||
| } else { | ||
| excerptString = tempExcerptString[ 1 ] | ||
| } |
Contributor
There was a problem hiding this comment.
The tempExcerptString[ 1 ] errors out & I'm not sure why we need tempExcerptString, we can just use this directly:
Suggested change
| if ( tempExcerptString[ 1 ].length > ( excerptLength || 55 ) ) { | |
| excerptString = tempExcerptString[ 1 ].substring( 0, excerptLength === '' ? 55 : excerptLength ) + '...' | |
| } else { | |
| excerptString = tempExcerptString[ 1 ] | |
| } | |
| if ( postExcerptStackable.length > ( excerptLength || 55 ) ) { | |
| excerptString = postExcerptStackable.substring( 0, excerptLength === '' ? 55 : excerptLength ) + '...' | |
| } else { | |
| excerptString = postExcerptStackable | |
| } |
Contributor
Author
There was a problem hiding this comment.
const tempExcerptString = postExcerptStackable.match( '<p>(.*)<\/p>\\n' ) returns an array and the 1st element is not the match, that is why the index is 1. Though I am not sure if this is the best way to do it.
bfintal
requested changes
May 10, 2022
Contributor
bfintal
left a comment
bfintal
left a comment
There was a problem hiding this comment.
Please see comments. The PHP side gives the wrong substr string. The JS produces an error
bfintal
added
PR: Reviewed w/ Comments
prconcepcion
added
PR: Needs Review
|
Added this issue to Release Cycle Project since a user raised this issue. See https://www.facebook.com/groups/wpstackable/permalink/1195920727963105/ |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fixes #2203