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

Add Inline comment experimental flag #60622

Merged
merged 274 commits into from
Oct 23, 2024

Conversation

poojabhimani12
Copy link
Contributor

@poojabhimani12 poojabhimani12 commented Apr 10, 2024

What?

Introducing the "Block Comment" experiment, with added functionality accessible via this feature flag which when enabled, allows users to add comments to the selected block by clicking on "comment" icon in the toolbar dropdown.

Screenshot at May 14 12-01-47

Why?

#59445

How?

Testing Instructions

  1. Enable the Block Comment experiment
  2. Insert any block
  3. Click on the block settings dropdown from the block toolbar
  4. Click the comment icon in the toolbar and add comment.

Screenshots or screencast

Current state:

image

Earlier iterations:

Screenshot at May 14 12-11-59 Screenshot at May 14 12-13-15
demo.webm

Copy link

github-actions bot commented Apr 10, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: MD-sunilprajapati <[email protected]>
Co-authored-by: poojabhimani12 <[email protected]>
Co-authored-by: rishishah-multidots <[email protected]>
Co-authored-by: ingeniumed <[email protected]>
Co-authored-by: ellatrix <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: mtias <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: annezazu <[email protected]>
Co-authored-by: jameskoster <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Apr 10, 2024
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @poojabhimani12! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@poojabhimani12 poojabhimani12 marked this pull request as draft April 10, 2024 06:20
@poojabhimani12 poojabhimani12 marked this pull request as ready for review May 16, 2024 04:49
@cbravobernal cbravobernal added the [Type] Experimental Experimental feature or API. label May 16, 2024
@ingeniumed
Copy link
Contributor

The following is a summary of the discussion that was had with @ellatrix, that's being posted by me for posterity.

In order to be able to test this feature end to end, this PR would need to be updated to add the ability to save comments as well. As a result, the following will need to be be done as the next steps:

  • Update the PR with the data model proposed here so that it actually persists everything. That way, you can see it working end to end.
  • Once this PR is merged in, keep an eye on the feature to see what sort of feedback is coming. Make any changes relevant to the feedback.
  • After sometime, put up a PR to drop the experimental flag so that it can ship in the next WP release - ideally 6.7

@poojabhimani12 poojabhimani12 requested a review from ajitbohra as a code owner June 5, 2024 06:42
@poojabhimani12 poojabhimani12 marked this pull request as draft June 7, 2024 05:34
@poojabhimani12 poojabhimani12 marked this pull request as ready for review June 10, 2024 11:46
@poojabhimani12
Copy link
Contributor Author

The following is a summary of the discussion that was had with @ellatrix, that's being posted by me for posterity.

In order to be able to test this feature end to end, this PR would need to be updated to add the ability to save comments as well. As a result, the following will need to be be done as the next steps:

  • Update the PR with the data model proposed here so that it actually persists everything. That way, you can see it working end to end.
  • Once this PR is merged in, keep an eye on the feature to see what sort of feedback is coming. Make any changes relevant to the feedback.
  • After sometime, put up a PR to drop the experimental flag so that it can ship in the next WP release - ideally 6.7

I have updated the PR as proposed above. Please review it and provide any feedback.

Copy link
Contributor

@ingeniumed ingeniumed left a comment

Choose a reason for hiding this comment

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

Noticed while testing this using playground and wp-now that the block comments are enabled by default, regardless of the checkbox being selected or not. In addition, the collab sidebar is not visible either.

const post = select( editorStore ).getCurrentPost();

return {
threads: post?.meta?.collab ? JSON.parse( post.meta.collab ) : [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this create problem if there's a large enough number of comments on a post? Could it be lazy loaded in some way to work around this problem? Thinking in terms of google docs, you have to scroll both on the page and on the comments dropdown to see all the comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have implemented a feature where the sidebar displays if any comment is open. However, we have now changed the flow so that the sidebar will be visible even if there are no collaborative activities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have conducted testing and believe that a large number of comments will not have a significant impact. Once the flow is finalized, we can definitely implement lazy loading.

packages/format-library/package.json Outdated Show resolved Hide resolved
…enting

Remove 6.7 rest API and other minor fixes
},
];

const moreActions = actions.filter( ( item ) => item.onClick );
Copy link
Member

Choose a reason for hiding this comment

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

actions is constant, so this doesn't filter anything?

@@ -115,7 +115,6 @@ function gutenberg_override_default_rest_server() {
}
add_filter( 'wp_rest_server_class', 'gutenberg_override_default_rest_server', 1 );


Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this line to have a clean change set?

@ellatrix ellatrix dismissed ciampo’s stale review October 23, 2024 07:09

Feedback has been addressed.

status: 'any',
per_page: 100,
} );
return Array.isArray( data ) ? data : [];
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't create a new array inside useSelect, it will create excessive re-renders. useSelect will think there's change. Can you create an EMTPY_ARRY constant outside this component. Same for the early return above.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, let it return null and move the array checks outside of useSelect.

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Ok, there's a number of things that should be addressed in a follow-up, but let's start with this.

@ellatrix ellatrix merged commit a775b7c into WordPress:trunk Oct 23, 2024
63 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.6 milestone Oct 23, 2024
@mtias
Copy link
Member

mtias commented Oct 24, 2024

Kudos all for driving this one!

@andrewserong
Copy link
Contributor

Fantastic work here, this is so exciting to see! 🎉 I've just done a tiny update to the PR description and added another screenshot as I missed at first that the Comments button had been moved to the block settings menu. Hope you don't mind!

karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
Co-authored-by: MD-sunilprajapati <[email protected]>
Co-authored-by: poojabhimani12 <[email protected]>
Co-authored-by: rishishah-multidots <[email protected]>
Co-authored-by: ingeniumed <[email protected]>
Co-authored-by: ellatrix <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: mtias <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: annezazu <[email protected]>
Co-authored-by: jameskoster <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Experimental Experimental feature or API.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.