-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Feature: Collaborative Notes Pane for the Editor screen #7563
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
base: main
Are you sure you want to change the base?
Conversation
In case this also helps navigate changes. Here's a summary provided by CoderabbitAI on the files changed:
|
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.
Pull Request Overview
This PR introduces a collaborative Notes Pane feature for the Editor screen, enabling users to add, edit, resolve, and delete notes linked to entries in the Editorial Workflow. The notes are backed by GitHub PR comments and synchronized with backend storage.
- Added comprehensive Notes system with CRUD operations, resolution tracking, and UI components
- Implemented backend support for GitHub and Test backends with PR comments integration
- Added localization support for all note-related messages and actions
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
packages/decap-cms-locales/src/en/index.js | Added English localization strings for Notes Pane UI and notifications |
packages/decap-cms-lib-util/src/implementation.ts | Added Note interface and optional note-related methods to Implementation |
packages/decap-cms-core/src/types/redux.ts | Added Note types and action payload definitions for Redux state |
packages/decap-cms-core/src/reducers/entryDraft.js | Updated reducer to handle notes state and actions |
packages/decap-cms-core/src/components/Editor/EditorNotesPane/*.js | Created Notes Pane UI components for displaying and managing notes |
packages/decap-cms-core/src/components/Editor/EditorInterface.js | Integrated Notes Pane into editor interface with toggle functionality |
packages/decap-cms-core/src/components/Editor/Editor.js | Added notes loading and change handling logic |
packages/decap-cms-core/src/backend.ts | Added backend wrapper methods for note operations |
packages/decap-cms-core/src/actions/entries.ts | Implemented Redux actions for notes CRUD operations |
packages/decap-cms-backend-/src/implementation. | Added notes implementation for GitHub, Test, and Proxy backends |
state = { | ||
showEventBlocker: false, | ||
previewVisible: localStorage.getItem(PREVIEW_VISIBLE) !== 'false', | ||
notesVisible: localStorage.getItem(NOTES_VISIBLE) === 'false', |
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.
The logic for initializing notesVisible is inverted. It should be === 'true' to show notes when the localStorage value is 'true', not when it's 'false'.
notesVisible: localStorage.getItem(NOTES_VISIBLE) === 'false', | |
notesVisible: localStorage.getItem(NOTES_VISIBLE) === 'true', |
Copilot uses AI. Check for mistakes.
<AvatarImage | ||
src={note.get('avatarUrl')} | ||
alt={`${note.get('author')} avatar`} | ||
onError={e => { |
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.
The onError handler assumes the next sibling exists but doesn't verify it. This could cause runtime errors if the DOM structure changes. Consider adding a null check: e.target.nextSibling?.style && (e.target.nextSibling.style.display = 'flex')
Copilot uses AI. Check for mistakes.
author: currentUser.login || currentUser.name, | ||
avatarUrl: currentUser.avatar_url, | ||
entrySlug: slug, | ||
timestamp: noteData.timestamp || new Date().toISOString(), |
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.
The noteData.timestamp is being used as a fallback, but the Note interface shows timestamp as a required field. Either the interface should mark it as optional or this fallback should be removed to ensure consistency.
Copilot uses AI. Check for mistakes.
private formatNoteForPR(note: Note): string { | ||
const status = note.resolved ? API.NOTE_STATUS_RESOLVED : API.NOTE_STATUS_OPEN; | ||
|
||
return `<!-- DecapCMS Note - Status: ${status} --> |
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.
[nitpick] The template literal has trailing spaces after the HTML comment. This could cause inconsistent formatting. Consider removing trailing whitespace or using a more structured approach.
return `<!-- DecapCMS Note - Status: ${status} --> | |
return `<!-- DecapCMS Note - Status: ${status} --> |
Copilot uses AI. Check for mistakes.
const loadedEntry = await tryLoadEntry(getState(), collection, slug); | ||
dispatch(entryLoaded(collection, loadedEntry)); | ||
dispatch(createDraftFromEntry(loadedEntry)); | ||
await dispatch(loadNotes(collection, slug)); |
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.
The loadNotes call is made without checking if the backend supports notes or if workflow is enabled. This could cause unnecessary API calls or errors for backends that don't implement notes.
Copilot uses AI. Check for mistakes.
handleBlur = () => { | ||
// Delay to allow button click to register | ||
setTimeout(() => { | ||
if (!this.state.content.trim()) { | ||
this.setState({ isFocused: false }); | ||
} | ||
}, 150); |
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.
Using setTimeout with magic number 150ms to handle blur/click timing is fragile and could cause race conditions. Consider using a more robust approach like checking if the click target is the submit button or using onMouseDown instead of onClick.
handleBlur = () => { | |
// Delay to allow button click to register | |
setTimeout(() => { | |
if (!this.state.content.trim()) { | |
this.setState({ isFocused: false }); | |
} | |
}, 150); | |
handleBlur = e => { | |
const { relatedTarget } = e; | |
if (relatedTarget && relatedTarget.type === 'submit') { | |
return; // Do not update state if blur is caused by clicking the submit button | |
} | |
if (!this.state.content.trim()) { | |
this.setState({ isFocused: false }); | |
} |
Copilot uses AI. Check for mistakes.
Hey @martinjagodic, I'm planning to tackle some of the action items from Copilot and to review this and ensure it is production-ready for the Github Backend Leaving the base for others to add compatibility with the other backends if they are interested. The action items I'm thinking of before moving this from Draft PR to Ready are:
I believe that with that done, I would consider it ready to be implemented in Main after your review. Let me know your thoughts on this and if you'd like to add anything else, or if you have any feedback. Thanks very much! |
@EstoesMoises sounds good! How do you debug this while developing, as it doesn't work with the local server? |
@martinjagodic I've been swapping backends by changing the config.yml from the dev-test folder. Since I'm focusing on Github compatibility, I'm using the configuration for Github locally - https://github.com/decaporg/decap-cms/blob/main/dev-test/config.yml For debugging, I created and connected my GitHub account and I use a GitHub repository I made for this purpose. Please let me know if there is a different approach that you suggest. Side note: So far, I've tested the Test Backend and GitHub Backend. When I started implementing this feature, I also considered creating functionality for the local backend by creating a .json for notes, but I reverted those changes to make this feature only available on Editorial Workflow. |
I had a really quick check and these are my first impressions:
|
Perfect! - Thanks for the feedback Martin. I will work on this |
Just wanted to leave a quick note - due to some unexpected life events, it might take me up to two weeks to get back with an update. I’ll still be working on the changes as soon as I can and I'm looking forward and aiming to get this feature completed 👍 |
Hello everyone, due to unexpected circumstances, it has been a while since my last update, but I'm still here and will get this updated very soon 👍 I'm just sending this message to confirm that this is not abandoned, and I'm very excited about getting this feature ready :P |
…hed and unpublished entries
Hey @martinjagodic this PR is ready for your review.
Let me know your thoughts on this ! Looking forward to your feedback |
Hello everyone! I'm super happy to be writing this PR as it's one of my first open-source contributions :) Plus, this is a feature that I really want to have as part of DecapCMS.
Meet the Notes Pane!
Summary
Currently, the Notes feature has only been fully implemented on the GitHub backend and the Test Backend. I've also written some code for the Proxy Backend, but the feature will not work with
local_backend
as it requires the Editorial Workflow.In essence, the goal of this feature works as a layer of abstraction for PR issue comments. With this we take more advantage of the information that is recorded in each PR issue. This aligns well with the philosophy behind the Editorial Workflow, particularly the (awesome) work Decap has done around PR and branch handling.
I made this change after following the discussion on [Issue 52](#52), and personally Decap is a tool that I really want to use for some Documentation projects, the only thing I felt was missing was a "comments" feature, which is collaboration aspect that other popular platforms like Google Docs provide. But well, Decap is cooler ¯_(ツ)_/¯ so it should have one as well.
Test Plan / Demonstration
Notes Pane UI within the Editor:
GitHub Comments:
Resolving Notes adds an HTML comment to the GitHub PR Issue Comment:
This passes all current tests, and I've modified some test files to account for the Notes actions. There’s still a pending change to add backend/implementation-specific testing.
Current state / room for improvement
There is definitely room for improvement in this MVP. A few features that come to mind, and that are definitely on my roadmap (and I’d be more than happy to collaborate with others to make them happen) are:
One of the main reasons I wanted to publish this in its current state is to get some early feedback before diving into more work. I’m really looking forward to hearing the maintainers’ thoughts and collaborating to make the Notes Pane feature as solid as possible.
Checklist
Please add an
x
inside each checkbox:A picture of a cute animal (not mandatory but encouraged)
My dog, Luna :)