-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: [FC-0044] Course unit - Copy/paste functionality #884
feat: [FC-0044] Course unit - Copy/paste functionality #884
Conversation
Thanks for the pull request, @PKulkoRaccoonGang! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #884 +/- ##
==========================================
- Coverage 92.14% 92.02% -0.12%
==========================================
Files 637 686 +49
Lines 11149 11955 +806
Branches 2407 2571 +164
==========================================
+ Hits 10273 11002 +729
- Misses 847 917 +70
- Partials 29 36 +7 ☔ View full report in Codecov by Sentry. |
af87165
to
5435ce0
Compare
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
@arbrandes The initial 1 commit in this PR is temporary. Once the #882 is merged, the third commit of this PR will become the main commit. |
Bug. 3: I can't get the "Paste component" button to appear. I can see there is code for it but when I copy a component, nothing happens. (It does copy to the clipboard, as I can verify using the old unit page, but no button appears in the new MFE.) No error appears in the console. |
@bradenmacdonald thanks for your contribution 💯
Now we can notice the use of two different approaches to working with BroadcastChannel. I have two options for solving these differences. Tomorrow after a discussion with the team I will provide you with more information.
Yes, you are right. Now we are not able to use the BroadcastChannel (between MFE and Legacy) without reloading the page, but for testing, after copying an element in MFE, you can reload the page in Legacy.
Have you tried seeing the button after copying a unit? |
Yes, that works (the button appears at the top of the page). But when I copy a component (XBlock, not a unit), the button at the bottom of the page does not appear for me. |
@PKulkoRaccoonGang OK, the reason that the "paste component" button was not working on my devstack is it was missing this commit: raccoongang/edx-platform@b686d70 Is there a PR to merge that to master edx-platform? |
@bradenmacdonald Yes, this PR is not yet available in the upstream. I recommend using a sandbox (or the branch specified in the sandbox deployment configuration) for testing. This way you will have the most current version of the platform.
After a discussion with the team, we decided to fix and check the communication of the Copy/paste functionality between the Course outline and Course unit pages and refactor (move elements that are repeated on both pages into generics). In order not to block the flow of delivery of new features, we propose to do this within a separate PR, which we will later open after development and testing. |
#882 is merged. @PKulkoRaccoonGang, mind rebasing? Probably good to upgrade the PR off of Draft, too. |
Or rather, let's leave it in draft until it doesn't depend on any other PRs. Is there one for raccoongang/edx-platform@b686d70, yet? |
5435ce0
to
14eba02
Compare
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
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.
Thanks! This seems much nicer but unfortunately the "paste component" button is still not showing up for me. I'm not sure why?
The "paste unit" appears to be working correctly.
xblock-clipboard-missing.mov
Also, it seems to be missing some kind of notification that the copy worked (or not)?
package.json
Outdated
@@ -121,5 +120,8 @@ | |||
}, | |||
"peerDependencies": { | |||
"decode-uri-component": ">=0.2.2" | |||
}, | |||
"overrides": { | |||
"react-intl": "^6.4.0" |
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.
Did you mean to include this override here? I don't think we should be specifying the react-intl
version as it comes from frontend-platform
.
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.
Agree that better to use react-intl
from the frontend-platform
. Let's remove it and look that all CI checks were successful.
src/constants.js
Outdated
error: 'error', | ||
}; | ||
|
||
export const NOT_XBLOCK_TYPES = ['vertical', 'sequential', 'chapter', 'course']; |
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 don't think the name of this variable is very clear. Technically, those are XBlocks. Maybe NOT_COMPONENT_TYPES
or STRUCTURAL_XBLOCK_TYPES
or add a comment with more details about what this is?
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.
Renamed to STRUCTURAL_XBLOCK_TYPES
src/course-unit/CourseUnit.test.jsx
Outdated
}); | ||
|
||
await executeThunk(fetchCourseSectionVerticalData(blockId), store.dispatch); | ||
await executeThunk(copyToClipboard(blockId), store.dispatch); |
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.
Why do you have to manually call these thunks in the test case? I would have expected that the line above, userEvent.click(getByRole('button', { name: ...CopyToClipboard... }));
will trigger the thunks.
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.
It was an extra call of executeThunk
and was removed, thanks
const messages = defineMessages({ | ||
hasConflictingErrorsTitle: { | ||
id: 'course-authoring.course-unit.paste-notification.has-conflicting-errors.title', | ||
defaultMessage: 'Files need to be updated manually.', |
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 believe all messages should now have a description
in our MFE codebases.
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.
Descriptions were added
src/course-unit/constants.js
Outdated
ready: 'ready', | ||
expired: 'expired', | ||
error: 'error', | ||
}; |
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.
These constants are all duplicates of the ones in src/constants.js
- we should only define them once.
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.
Totally agree, removed an extra constant
src/__mocks__/clipboardUnit.js
Outdated
@@ -0,0 +1,16 @@ | |||
module.exports = { |
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.
module.exports = { | |
export default { |
Try to avoid module.exports
if at all possible.
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.
Changed
src/__mocks__/clipboardXBlock.js
Outdated
@@ -0,0 +1,16 @@ | |||
module.exports = { |
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.
module.exports = { | |
export default { |
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.
Changed
|
||
const handleKeyDown = ({ key }) => { | ||
if (key === 'Tab') { | ||
popoverElementRef.current.focus(); |
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.
popoverElementRef.current.focus(); | |
popoverElementRef.current?.focus(); |
I got an error here when testing the course outline page pressing TAB and SHIFT-TAB to move focus around via keyboard:
react-dom.development.js:4091 Uncaught TypeError: Cannot read properties of null (reading 'focus')
at handleKeyDown (WhatsInClipboard.jsx:17:33)
at HTMLUnknownElement.callCallback (react-dom.development.js:3945:14)
at Object.invokeGuardedCallbackDev (react-dom.development.js:3994:16)
at invokeGuardedCallback (react-dom.development.js:4056:31)
at invokeGuardedCallbackAndCatchFirstError (react-dom.development.js:4070:25)
at executeDispatch (react-dom.development.js:8243:3)
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.
Fixed, thanks
<PasteButton className={className} onClick={onClick} text={text} /> | ||
<OverlayTrigger | ||
show={showPopover} | ||
trigger={OVERLAY_TRIGGERS} |
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 have <OverlayTrigger trigger={OVERLAY_TRIGGERS}
and OVERLAY_TRIGGERS = ['hover', 'focus']
then why do <WhatsInClipboard>
and the <Popover>
above both have their own onMouseEnter
, onMouseLeave
, onFocus
, onBlur
handlers? There seems to be a lot of custom code here just to display a tooltip, and it seems a bit buggy in my testing. Can we rely more on Paragon and use less custom code here? I feel like there should already be a reusable interactive tooltip widget that doesn't require writing your own hover/focus handlers.
For reference, the legacy implementation doesn't use any JavaScript at all - just the :hover
and :focus-within
styles to make it appear.
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 agree that it looks with excessive logic and processing, but this was done to keep the tooltip in an open state and the ability to click on it, since the entire body of the tooltip is a link to the unit from which the component was copied
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 agree with @ihor-romaniuk. A generic tooltip is not supposed to be used when there is a clickable link inside. The alternative to a tooltip is a ModalPopover
, but it was decided not to use that because of the z-indexing and because it requires the user to click when we want the open state to be available on hover.
@@ -0,0 +1,9 @@ | |||
module.exports = { |
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.
module.exports = { | |
export default { |
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.
Changed
<Dropdown.Item onClick={() => dispatch(copyToClipboard(id))}> | ||
{intl.formatMessage(messages.blockLabelButtonCopyToClipboard)} | ||
</Dropdown.Item> | ||
)} |
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.
Duplicate of the previous comment
a8b2baa
to
7cb96ce
Compare
This is due to the lack of necessary changes on the edx-platform side, namely openedx/edx-platform#34521 We have to wait for it to merge before merging this PR. Thanks for notice that.
If you mean alert messages that appear after pasting a copied unit, then they have been implemented. I have attached the screenshots below. |
Sandbox deployment successful 🚀 |
Sandbox deployment successful 🚀 |
No, I mean a notification that the copy worked. (The legacy UI doesn't have this but it does display a "Copying..." notification while the copy is happening.) When I tested this, it was hard to tell that the Copy button did anything because there was no change after clicking it. |
Sandbox deployment failed 💥 |
Ah, I see. This is another area where we shouldn't have copied the old design. A "Copying..." status message that is barely even visible doesn't really help much. We always wanted to display "Copied!" toast popup after the copy succeeds, but the legacy UI had no API for displaying a "Copied!" success message. But in the MFE, with Paragon, it should be very easy to display a "Copied!" success @jmakowski1123 could you advise? |
* feat: [AXIMST-344] Copy/paste functionality base * feat: [AXIMST-344] Copy/paste functionality visible part * feat: tests * fix: PR comment review * refactor: refactoring after review * refactor: refactoring after rebase --------- Co-authored-by: monteri <lansevermore> Co-authored-by: PKulkoRaccoonGang <[email protected]> feat: [AXIMST-375] Course unit - Added functionality for copying and pasting xblocks and units (#147) * feat: [AXIMST-350] added functionality for copying and pasting xblocks and units * refactor: refactoring after review * refactor: refactoring after second review fix: [AXIMST-480] fixed paste notification behavior after switching a unit (#160) fix: [AXIMST-478] fixed copy-paste tooltip (#161) feat: [AXIMST-338] Course unit - Added canEdit and canPasteComponent variables (#170) * feat: [AXIMST-338] added canEdit and canPasteComponent variables * refactor: added condition for Can copy Unit btn feat: [AXIMST-525] separated the copy unit button (#190) refactor: [AXIMST-507] Course unit - Changed Paste unit UI (#186) * refactor: [AXIMST-507] changed Paste unit UI * refactor: code refactoring fix: fixed react-intl error (#197) fix: [AXIMST-516] fixed paste alerts view (#189) refactor: code refactoring refactor: code refactoring
* refactor: copy paste functional refactoring * refactor: refactoring paste-button * refactor: tests refactoring * refactor: updated translations * refactor: refactoring after review * refactor: renamed status selector name
* fix: [AXIMST-718] second attempt to fix the CSRF token * fix: [AXIMST-718] third attempt to fix the CSRF token * fix: [AXIMST-767] fixed new files alert * refactor: unnecessary code removed
2c1864e
to
e5ba0f7
Compare
Sandbox deployment successful 🚀 |
@bradenmacdonald We will bring up this point on the next project sync up. However, in any case we would really like to add this change in the follow-up PR, so we could merge this one and unblock dependent Unit page PRs. |
@GlugovGrGlib Oh yeah, that can definitely be addressed in a follow-up PR. Sorry for not being clear. |
I'm not planning to review this further - I'm not a CC on this repo in any case. I've mentioned the minor concerns that I have, which don't need to block anything. I'm assuming you have someone else to do the full review and merge? |
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.
Just missing descriptions in one small messages file. Otherwise, can't find anything to object to that hadn't been already addressed or pushed to a subsequent PR. Nice work!
const messages = defineMessages({ | ||
popoverContentText: { | ||
id: 'course-authoring.generic.paste-component.popover.content.text', | ||
defaultMessage: 'From:', |
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.
Can we get descriptions in this file, 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.
Descriptions were added. Please recheck and LGTM.
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.
Looks great, thank you!
Sandbox deployment failed 💥 |
@PKulkoRaccoonGang 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
1 similar comment
@PKulkoRaccoonGang 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Settings
Description
In this PR, the opportunity for the user to copy and insert xblocks and units was added.
The primary features were implemented:
Issue: openedx/platform-roadmap#321
Developer notes
Testing instructions
contentstore.new_studio_mfe.use_new_unit_page
in the CMS admin panel.ENABLE_UNIT_PAGE=true
is enabled.contentstore.enable_copy_paste_units
a waffle flag in the CMS admin panel.Screen.Recording.2024-03-10.at.17.08.32.mov