-
Notifications
You must be signed in to change notification settings - Fork 195
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
Show notice in Course Editor to focus on Course Outline block or Insert one if block does not exist for first time users #7364
Open
Imran92
wants to merge
30
commits into
trunk
Choose a base branch
from
add/notice-to-create-course-outline-block-first-time
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
9d77b03
Show notice to create outline, or focus outline first time
Imran92 b556656
Add import comments
Imran92 d328e85
Add notice file in webpack
Imran92 4dcfa9d
Enqueue first course notice only when first course
Imran92 8325305
Test published lesson checking function
Imran92 bfdcdb5
Add tests for conditionally creating or selecting outline block
Imran92 b1b0d13
Update logic to show notice
Imran92 324e6d2
Add changelog
Imran92 af681c6
Update the comment
Imran92 ddb29ea
Only show notice after sensei pattern inserted
Imran92 f424669
Show notice only when no course of any status exists
Imran92 d840ca2
Merge branch 'trunk' into add/notice-to-create-course-outline-block-f…
Imran92 88ee206
Merge branches 'add/notice-to-create-course-outline-block-first-time'…
Imran92 12bd25d
Move pattern variable check inside function
Imran92 4bc987c
Show notice when there is no published lesson
Imran92 7460bd6
Fix test
Imran92 1cfec71
Merge branch 'trunk' into add/notice-to-create-course-outline-block-f…
donnapep 1a7aba8
Merge branch 'trunk' into add/notice-to-create-course-outline-block-f…
Imran92 c43f79c
Fix failing tests
Imran92 e84c62a
Show notice only when no lessons are created
Imran92 f749ad7
Wait for pattern on new course, else show right away
Imran92 3d82cd5
change name of hasOutlineBlock function
Imran92 05099e6
Add meta for new post for course
Imran92 ee75e87
Remove inserter pattern selection detection code
Imran92 a633f08
Manually select outline block only when available
Imran92 13e2d98
Remove unused import
Imran92 1166860
Merge branch 'trunk' into add/notice-to-create-course-outline-block-f…
Imran92 3d47a01
Remove unnecessary async
Imran92 5e5e764
Set default value to null
Imran92 ad3aeee
Set default to false
Imran92 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { createBlock } from '@wordpress/blocks'; | ||
import { select, subscribe, dispatch } from '@wordpress/data'; | ||
import domReady from '@wordpress/dom-ready'; | ||
import { __ } from '@wordpress/i18n'; | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import { getFirstBlockByName } from '../../blocks/course-outline/data'; | ||
|
||
export const getOutlineBlock = () => | ||
getFirstBlockByName( | ||
'sensei-lms/course-outline', | ||
select( 'core/block-editor' ).getBlocks() | ||
); | ||
|
||
export const handleCourseOutlineBlockIncomplete = () => { | ||
const courseOutlineBlock = getOutlineBlock(); | ||
|
||
// If the course outline block exists, just select it. | ||
if ( courseOutlineBlock ) { | ||
dispatch( 'core/editor' ).selectBlock( courseOutlineBlock.clientId ); | ||
return; | ||
} | ||
|
||
const { insertBlock } = dispatch( 'core/block-editor' ); | ||
|
||
insertBlock( createBlock( 'sensei-lms/course-outline' ) ); | ||
}; | ||
|
||
// If the function isn't globally available, the link button doesn't find the reference. | ||
window.handleCourseOutlineBlockIncomplete = handleCourseOutlineBlockIncomplete; | ||
|
||
export const hasLessonInOutline = ( blocks ) => { | ||
return blocks.some( ( block ) => { | ||
if ( block.name === 'sensei-lms/course-outline-lesson' ) { | ||
return true; | ||
} | ||
|
||
if ( block.innerBlocks?.length ) { | ||
return hasLessonInOutline( block.innerBlocks ); | ||
} | ||
|
||
return false; | ||
} ); | ||
}; | ||
|
||
export const handleFirstCourseCreationHelperNotice = () => { | ||
const { createInfoNotice, removeNotice } = dispatch( 'core/notices' ); | ||
const userId = select( 'core' ).getCurrentUser()?.id; | ||
const { getEditedPostAttribute } = select( 'core/editor' ); | ||
const { editPost } = dispatch( 'core/editor' ); | ||
const firstCourseNoticeDismissedKey = | ||
'sensei-lms-first-course-notice-dismissed-' + userId; | ||
const isFirstCourseNoticeDismissed = !! window.localStorage.getItem( | ||
firstCourseNoticeDismissedKey | ||
); | ||
const noticeId = 'course-outline-block-setup-incomplete'; | ||
const isNewCourse = window?.sensei?.isNewCourse; | ||
|
||
let noticeCreated = false; | ||
let noticeRemoved = false; | ||
|
||
const notice = __( | ||
'Nice! Now you can <a href="javascript:;" onclick="window?.handleCourseOutlineBlockIncomplete();">add some lessons</a> to your course.', | ||
'sensei-lms' | ||
); | ||
|
||
let isNewPostMetaSet = false; | ||
|
||
subscribe( () => { | ||
if ( isNewCourse && ! isNewPostMetaSet ) { | ||
if ( getOutlineBlock() ) { | ||
isNewPostMetaSet = true; | ||
editPost( { | ||
meta: { _new_post: true }, | ||
} ); | ||
} | ||
} | ||
const patternSelected = | ||
! isNewCourse || | ||
( isNewPostMetaSet && | ||
false === getEditedPostAttribute( 'meta' )?._new_post ); | ||
if ( | ||
noticeCreated && | ||
! noticeRemoved && | ||
getOutlineBlock() && | ||
hasLessonInOutline( [ getOutlineBlock() ] ) | ||
) { | ||
noticeRemoved = true; | ||
noticeCreated = false; | ||
removeNotice( noticeId ); | ||
} | ||
|
||
// If the user selects a Sensei pattern or editing an existing Course, and the notice hasn't been created, and notice hasn't been dismissed, and either the course outline block hasn't been created OR there are no published lessons in the outline, create the notice. | ||
if ( | ||
patternSelected && | ||
! noticeCreated && | ||
! isFirstCourseNoticeDismissed && | ||
! ( | ||
getOutlineBlock() && hasLessonInOutline( [ getOutlineBlock() ] ) | ||
) | ||
) { | ||
noticeCreated = true; | ||
noticeRemoved = false; | ||
|
||
createInfoNotice( notice, { | ||
id: noticeId, | ||
isDismissible: true, | ||
__unstableHTML: true, // Necessary to render the link in the middle of the notice message. | ||
onDismiss: () => { | ||
window.localStorage.setItem( | ||
firstCourseNoticeDismissedKey, | ||
'1' | ||
); | ||
}, | ||
} ); | ||
} | ||
} ); | ||
}; | ||
|
||
// Call function on dom ready. | ||
domReady( handleFirstCourseCreationHelperNotice ); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { createBlock } from '@wordpress/blocks'; | ||
import { dispatch, select, subscribe } from '@wordpress/data'; | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import { | ||
getOutlineBlock, | ||
handleCourseOutlineBlockIncomplete, | ||
handleFirstCourseCreationHelperNotice, | ||
hasLessonInOutline, | ||
} from './first-course-creation-notice'; | ||
import { getFirstBlockByName } from '../../blocks/course-outline/data'; | ||
|
||
// Initial mocks. | ||
jest.mock( '@wordpress/blocks', () => ( { | ||
createBlock: jest.fn().mockImplementation( () => ( { | ||
attributes: {}, | ||
clientId: 'new-block-id', | ||
} ) ), | ||
} ) ); | ||
|
||
jest.mock( '@wordpress/data', () => ( { | ||
dispatch: jest.fn().mockImplementation( () => ( { | ||
createInfoNotice: jest.fn(), | ||
removeNotice: jest.fn(), | ||
insertBlock: jest.fn(), | ||
selectBlock: jest.fn(), | ||
} ) ), | ||
select: jest.fn().mockImplementation( () => ( { | ||
getCurrentUser: jest.fn(), | ||
getBlocks: jest.fn(), | ||
} ) ), | ||
subscribe: jest.fn(), | ||
use: jest.fn(), | ||
} ) ); | ||
|
||
jest.mock( '@wordpress/i18n', () => ( { | ||
...jest.requireActual( '@wordpress/i18n' ), | ||
__: jest.fn(), | ||
} ) ); | ||
|
||
jest.mock( './first-course-creation-notice' ); | ||
jest.mock( '../../blocks/course-outline/data' ); | ||
|
||
describe( 'hasPublishedLessonInOutline', () => { | ||
beforeEach( () => { | ||
hasLessonInOutline.mockImplementation( | ||
jest.requireActual( './first-course-creation-notice' ) | ||
.hasLessonInOutline | ||
); | ||
} ); | ||
|
||
it( 'should return true when there is a lesson in the outline', () => { | ||
const blocks = [ | ||
{ | ||
name: 'sensei-lms/course-outline-lesson', | ||
}, | ||
]; | ||
|
||
const result = hasLessonInOutline( blocks ); | ||
|
||
expect( result ).toBe( true ); | ||
} ); | ||
|
||
it( 'should return false when there is no lesson in the outline', () => { | ||
const blocks = [ { name: 'some-other-block' } ]; | ||
|
||
const result = hasLessonInOutline( blocks ); | ||
|
||
expect( result ).toBe( false ); | ||
} ); | ||
} ); | ||
|
||
describe( 'handleCourseOutlineBlockIncomplete', () => { | ||
beforeEach( () => { | ||
handleCourseOutlineBlockIncomplete.mockImplementation( | ||
jest.requireActual( './first-course-creation-notice' ) | ||
.handleCourseOutlineBlockIncomplete | ||
); | ||
getFirstBlockByName.mockClear(); | ||
getOutlineBlock.mockClear(); | ||
createBlock.mockClear(); | ||
} ); | ||
it( 'should create and insert a block when no course outline block exists', () => { | ||
// Mock hasOutlineBlock to return falsy. | ||
getFirstBlockByName.mockImplementation( () => null ); | ||
const mockInsertBlock = jest.fn(); | ||
dispatch.mockImplementation( () => ( { | ||
insertBlock: mockInsertBlock, | ||
selectBlock: jest.fn(), | ||
} ) ); | ||
|
||
handleCourseOutlineBlockIncomplete(); | ||
|
||
// Ensure createBlock and insertBlock were called with the correct parameters. | ||
expect( createBlock ).toHaveBeenCalledWith( | ||
'sensei-lms/course-outline' | ||
); | ||
expect( mockInsertBlock ).toHaveBeenCalled(); | ||
} ); | ||
|
||
it( 'should focus on the existing course outline block when it exists', () => { | ||
// Mock hasOutlineBlock to return a truthy value. | ||
getFirstBlockByName.mockImplementation( () => ( { | ||
clientId: 'existing-block-id', | ||
} ) ); | ||
const mockInsertBlock = jest.fn(); | ||
const mockSelectBlock = jest.fn(); | ||
dispatch.mockImplementation( () => ( { | ||
insertBlock: mockInsertBlock, | ||
selectBlock: mockSelectBlock, | ||
} ) ); | ||
|
||
handleCourseOutlineBlockIncomplete(); | ||
|
||
// Ensure selectBlock was called with the correct parameters. | ||
expect( mockSelectBlock ).toHaveBeenCalledWith( 'existing-block-id' ); | ||
// Ensure createBlock and insertBlock were not called. | ||
expect( createBlock ).not.toHaveBeenCalled(); | ||
expect( mockInsertBlock ).not.toHaveBeenCalled(); | ||
} ); | ||
} ); |
4 changes: 4 additions & 0 deletions
4
changelog/add-notice-to-create-course-outline-block-first-time
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Significance: minor | ||
Type: added | ||
|
||
Notice on Course editor for first course of a user guiding them to use Course Outline block properly |
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
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
Oops, something went wrong.
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.
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.
My suggestion wouldn't match the design, but it would match the default behavior of the Gutenberg notices.
Should we tweak it to use a separate button, being the
actions
from the Gutenberg notice instead? cc @donnapepSo we wouldn't need to put the function in the
window
, we wouldn't need to add theonclick
andhref="javascript:;"
as HTML attributes, it would get more organized in terms of code, and the users would the same behavior of other notices.If we decide to not change it, we need to extract the HTML and logic from the translation string.
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.
If we're good with the design change, we can go for it 👍 Our current design of showing it as a URL looks better to me. But as showing it as button is simpler, as that's the default behavior. I think the main benefit of this will be not having to use the
__unstableHTML
property.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 fine with changing it to use a button if it helps simplify things. In that case, we could change the message to:
where Add Lessons is a button that jumps to / adds the Course Outline block.
Let's see what that looks like. 🙂
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.
Updated comment here 🙂