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 🚀 |
bradenmacdonald
left a comment
There was a problem hiding this comment.
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)?
| "decode-uri-component": ">=0.2.2" | ||
| }, | ||
| "overrides": { | ||
| "react-intl": "^6.4.0" |
There was a problem hiding this comment.
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.
Agree that better to use react-intl from the frontend-platform. Let's remove it and look that all CI checks were successful.
| error: 'error', | ||
| }; | ||
|
|
||
| export const NOT_XBLOCK_TYPES = ['vertical', 'sequential', 'chapter', 'course']; |
There was a problem hiding this comment.
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.
Renamed to STRUCTURAL_XBLOCK_TYPES
| }); | ||
|
|
||
| await executeThunk(fetchCourseSectionVerticalData(blockId), store.dispatch); | ||
| await executeThunk(copyToClipboard(blockId), store.dispatch); |
There was a problem hiding this comment.
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.
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.
I believe all messages should now have a description in our MFE codebases.
There was a problem hiding this comment.
Descriptions were added
| ready: 'ready', | ||
| expired: 'expired', | ||
| error: 'error', | ||
| }; |
There was a problem hiding this comment.
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.
Totally agree, removed an extra constant
| @@ -0,0 +1,16 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
| module.exports = { | |
| export default { |
Try to avoid module.exports if at all possible.
| @@ -0,0 +1,16 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
| module.exports = { | |
| export default { |
|
|
||
| const handleKeyDown = ({ key }) => { | ||
| if (key === 'Tab') { | ||
| popoverElementRef.current.focus(); |
There was a problem hiding this comment.
| 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)
| <PasteButton className={className} onClick={onClick} text={text} /> | ||
| <OverlayTrigger | ||
| show={showPopover} | ||
| trigger={OVERLAY_TRIGGERS} |
There was a problem hiding this comment.
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.
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.
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.
| module.exports = { | |
| export default { |
| <Dropdown.Item onClick={() => dispatch(copyToClipboard(id))}> | ||
| {intl.formatMessage(messages.blockLabelButtonCopyToClipboard)} | ||
| </Dropdown.Item> | ||
| )} |
There was a problem hiding this comment.
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/openedx-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 <peter.kulko@raccoongang.com> 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? |
arbrandes
left a comment
There was a problem hiding this comment.
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.
Can we get descriptions in this file, please?
There was a problem hiding this comment.
Descriptions were added. Please recheck and LGTM.
arbrandes
left a comment
There was a problem hiding this comment.
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_pagein the CMS admin panel.ENABLE_UNIT_PAGE=trueis enabled.contentstore.enable_copy_paste_unitsa waffle flag in the CMS admin panel.Screen.Recording.2024-03-10.at.17.08.32.mov