fix: remove unnecessary toast notification on adding component #1490

navinkarkera · 2024-11-08T11:30:31Z

Fix for: #1489

Private-ref: FAL-3922

openedx-webhooks · 2024-11-08T11:30:35Z

Thanks for the pull request, @navinkarkera!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

navinkarkera · 2024-11-08T12:05:31Z

It seems like many of library tests are failing after merge of this commit:f9ef00e

codecov · 2024-11-08T12:40:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.02%. Comparing base (9b4cf87) to head (efe8d52).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1490   +/-   ##
=======================================
  Coverage   93.01%   93.02%           
=======================================
  Files        1048     1048           
  Lines       20480    20479    -1     
  Branches     4402     4401    -1     
=======================================
+ Hits        19049    19050    +1     
+ Misses       1361     1359    -2     
  Partials       70       70

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bradenmacdonald · 2024-11-08T19:29:58Z

@navinkarkera That fixed the bug, but now I am seeing weird behavior where a blank editor opens up after I save the new component.

New.Bug.mov

Also, the changes I made ("This is the new text") don't seem to have been saved.

bradenmacdonald · 2024-11-08T19:34:45Z

Hmm, I cleared my cache and the bug is slightly different - now it does save changes and the second editor only flickers briefly on the screen. But still definitely a bug with the editor appearing again:

New.bug.2.mov

navinkarkera · 2024-11-09T05:37:03Z

@bradenmacdonald I think it is the same issue that I posted here. I'll investigate and see if I can fix it here.

Update: The linkComponent function that links a component to collection is responsible for this bug.

navinkarkera · 2024-11-11T07:09:15Z

It is fixed now, had to link component after editor closing. I believe it will also help when we work on #1482 as we can trigger creation of new block on editor close using the same function.

Link component to collection after closing editor fix re-render of editor in collections page on link mutation.

pomegranited

👍 Thank you for this fix, and for cleaning up and improving tests @navinkarkera !

I tested this by ensuring that no toast is shown with the editor modal when adding new components to a library or collection. Also tested that the toast is still shown when pasting components into a library or collection, and that the "Content linked successfully" toast and message have been removed.
I read through the code
I checked for accessibility issues by using my keyboard to navigate
Improves the code documentation
User-facing strings are extracted for translation

pomegranited · 2024-11-12T00:42:01Z

src/library-authoring/common/context.tsx

+  /** If the editor is open and the user is editing some component, this is its usageKey
+  and the callback function that should be called on closing the editor.
+  */


nit: componentBeingEdited isn't a usageKey anymore:

Suggested change

/** If the editor is open and the user is editing some component, this is its usageKey

and the callback function that should be called on closing the editor.

*/

// If the editor is open and the user is editing some component, this is the component being edited.

pomegranited · 2024-11-12T00:43:35Z

src/library-authoring/common/context.tsx

+  and the callback function that should be called on closing the editor.
+  */
+  componentBeingEdited: ComponentEditorInfo | undefined;
+  openComponentEditor: (usageKey: string, onClose?: () => void) => void;


nit (continued): ...and the rest of this comment can go here:

Suggested change

openComponentEditor: (usageKey: string, onClose?: () => void) => void;

// If an onClose callback is provided, it will be called when the editor is closed.

openComponentEditor: (usageKey: string, onClose?: () => void) => void;

ChrisChV

@navinkarkera is this ready to merge?

navinkarkera · 2024-11-14T05:17:05Z

@ChrisChV Yes, thank you!

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 8, 2024

navinkarkera mentioned this pull request Nov 8, 2024

Bug: Link content toast notification should not be displaying #1489

Open

navinkarkera assigned navinkarkera and unassigned navinkarkera Nov 8, 2024

navinkarkera requested a review from bradenmacdonald November 8, 2024 12:18

navinkarkera marked this pull request as ready for review November 8, 2024 12:44

navinkarkera requested a review from a team as a code owner November 8, 2024 12:44

navinkarkera force-pushed the navin/remove-unnecessary-toast-notification branch from 13eec2a to 3f95ab6 Compare November 9, 2024 16:58

navinkarkera force-pushed the navin/remove-unnecessary-toast-notification branch 2 times, most recently from 1f0761c to 1f7c6ce Compare November 11, 2024 07:48

navinkarkera added 3 commits November 11, 2024 22:15

fix: remove unnecessary toast notification on adding component

731de99

test: mock getWaffleFlags api call to fix tests

f43310a

fix: new block editor refresh in collection page

8414925

Link component to collection after closing editor fix re-render of editor in collections page on link mutation.

navinkarkera force-pushed the navin/remove-unnecessary-toast-notification branch from efbad92 to 816dd4d Compare November 11, 2024 16:53

navinkarkera added 3 commits November 11, 2024 22:27

fix: tests

acd4fda

docs: update componentBeingEdited variable doc

4ea7195

refactor: add content container tests

2734338

navinkarkera force-pushed the navin/remove-unnecessary-toast-notification branch from 816dd4d to 2734338 Compare November 11, 2024 16:57

pomegranited approved these changes Nov 12, 2024

View reviewed changes

docs: apply review suggestions

efe8d52

ChrisChV approved these changes Nov 13, 2024

View reviewed changes

ChrisChV merged commit 033acc4 into openedx:master Nov 14, 2024
7 checks passed

ChrisChV deleted the navin/remove-unnecessary-toast-notification branch November 14, 2024 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove unnecessary toast notification on adding component #1490

fix: remove unnecessary toast notification on adding component #1490

navinkarkera commented Nov 8, 2024 •

edited

Loading

openedx-webhooks commented Nov 8, 2024 •

edited

Loading

navinkarkera commented Nov 8, 2024

codecov bot commented Nov 8, 2024 •

edited

Loading

bradenmacdonald commented Nov 8, 2024

bradenmacdonald commented Nov 8, 2024

navinkarkera commented Nov 9, 2024 •

edited

Loading

navinkarkera commented Nov 11, 2024

pomegranited left a comment

pomegranited Nov 12, 2024

pomegranited Nov 12, 2024

ChrisChV left a comment

navinkarkera commented Nov 14, 2024

	openComponentEditor: (usageKey: string, onClose?: () => void) => void;
	// If an onClose callback is provided, it will be called when the editor is closed.
	openComponentEditor: (usageKey: string, onClose?: () => void) => void;

fix: remove unnecessary toast notification on adding component #1490

fix: remove unnecessary toast notification on adding component #1490

Conversation

navinkarkera commented Nov 8, 2024 • edited Loading

openedx-webhooks commented Nov 8, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

navinkarkera commented Nov 8, 2024

codecov bot commented Nov 8, 2024 • edited Loading

Codecov Report

bradenmacdonald commented Nov 8, 2024

bradenmacdonald commented Nov 8, 2024

navinkarkera commented Nov 9, 2024 • edited Loading

navinkarkera commented Nov 11, 2024

pomegranited left a comment

Choose a reason for hiding this comment

pomegranited Nov 12, 2024

Choose a reason for hiding this comment

pomegranited Nov 12, 2024

Choose a reason for hiding this comment

ChrisChV left a comment

Choose a reason for hiding this comment

navinkarkera commented Nov 14, 2024

navinkarkera commented Nov 8, 2024 •

edited

Loading

openedx-webhooks commented Nov 8, 2024 •

edited

Loading

codecov bot commented Nov 8, 2024 •

edited

Loading

navinkarkera commented Nov 9, 2024 •

edited

Loading