-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Block: Post comment date]: Add link setting and block supports #35112
Conversation
Size Change: +5.63 kB (+1%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
Thanks for the Pull Request! 🙂 @gziolo I'm afraid I don't feel comfortable yet reviewing the code, could you take a look at it, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I don't feel comfortable yet reviewing the code, could you take a look at it, please?
No worries @SantosGuillamot. The code looks good. I haven't tested so I would appreciate some help on that front.
if ( isLink ) { | ||
commentDate = ( | ||
<a | ||
href="#comment-date-pseudo-link" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use an actual link here? In PHP it's possible with get_comment_link( $block->context['commentId'] );
. Would it work with useEntityProp( 'root', 'comment', 'link', commentId )
or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want it to be clickable in the editor? Is the issue where duplicate editors were opened fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we have used fake links because the editor would be opened inside the editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this: #30647
Co-authored-by: Greg Ziółkowski <[email protected]>
Description
Closes #30573.
How has this been tested?
datetime
attribute and in the text.Types of changes
Enhancement
Checklist:
*.native.js
files for terms that need renaming or removal).