fix(core): Fix Title in "Untitled was published" toast #7473
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
No changes to documentation |
ricokahler
previously approved these changes
Sep 6, 2024
| const event: any = useDocumentOperationEvent(documentId, documentType) | ||
| const prevEvent = useRef(event) | ||
| const paneRouter = usePaneRouter() | ||
| const {t} = useTranslation(structureLocaleNamespace) | ||
|
|
||
| if ( |
There was a problem hiding this comment.
this may be more react-esque if it was calculated. e.g.
const title = useMemo(() => {
// If title isn't set from document preview, use the title from the document pane value
if (
!documentTitleInfo.title &&
!titleError &&
!IGNORE_OPS.includes(event?.op) &&
typeof documentPaneValue.title === 'string' &&
event?.type === 'success'
) {
return documentPaneValue.title
}
return documentTitleInfo.title
}, [documentTitleInfo.title, titleError, event, documentPaneValue.title])
There was a problem hiding this comment.
👍 Thanks for that suggestion, I was going back and forth on the decision to memoize it.
There was a problem hiding this comment.
its not too important to memoize it since the value isn't an object reference and is cheap to calculate but it kinda feels better to useMemo than to have an IIFE e.g. const title = (() => {})() do the same thing. mutating the variable is the thing to avoid in react renders
There was a problem hiding this comment.
Yeah, I like the memo approach better. I went ahead and made that change.
|
Component Testing Report Updated Sep 6, 2024 7:11 PM (UTC) ✅ All Tests Passed -- expand for details
|
ricokahler
approved these changes
Sep 6, 2024
ricokahler
pushed a commit
that referenced
this pull request
Sep 6, 2024
* fix(core): Fix Title in "Untitled was published" toast * PR feedback: refactor to useMemo
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.
Description
This PR fixes a bug where a success message shows "Untitled was published" when publishing (and unpublishing) a document with a title. This is especially prevalent on documents with an
imagefield.It adds an additional check for a title in the current document title when the title is not available on the document preview title.
What to review
Ensure that the using the document pane value directly as a last resort for a title is a safe approach.
Testing
titleand then publish the documentNotes for release