feat: delete collection [FC-0062] #1333

navinkarkera · 2024-09-26T15:18:13Z

Description

Adds delete option to collection card menu. It displays a confirmation box before deleting and a toast with button to undo delete action for 10 seconds.

Supporting information

Addresses: Soft Delete a Collection #1295
Private-ref: FAL-3851

Testing instructions

Setup tutor with this PR and https://github.com/open-craft/tutor-contrib-meilisearch plugin
Create library and some collections in the library.
Use delete option in collection card menu and verify its functionality.
Use the undo button in the toast to restore collection.

Concerns

Adding the undo button in a toast doesn't seem the best option in terms of accessibility.

openedx-webhooks · 2024-09-26T15:18:17Z

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.

codecov · 2024-09-27T13:29:27Z

Codecov Report

Attention: Patch coverage is 95.89041% with 3 lines in your changes missing coverage. Please review.

Project coverage is 93.01%. Comparing base (75f937e) to head (26da705).
Report is 35 commits behind head on master.

Files with missing lines	Patch %	Lines
...rc/library-authoring/components/CollectionCard.tsx	90.62%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1333   +/-   ##
=======================================
  Coverage   93.00%   93.01%           
=======================================
  Files        1035     1035           
  Lines       19570    19630   +60     
  Branches     4157     4100   -57     
=======================================
+ Hits        18201    18258   +57     
- Misses       1304     1310    +6     
+ Partials       65       62    -3

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

src/generic/toast-context/index.tsx

pomegranited

@navinkarkera I agree with your a11y concerns here, and proposed a way to address them. If you don't agree, I think it's worth asking Product for advice here.

pomegranited · 2024-10-02T00:52:51Z

src/generic/processing-notification/index.jsx

 import { Settings as IconSettings } from '@openedx/paragon/icons';
 import { capitalize } from 'lodash';

-const ProcessingNotification = ({ isShow, title }) => (
+const ProcessingNotification = ({ isShow, title, action }) => (


Concerns: Adding the undo button in a toast doesn't seem the best option in terms of accessibility.

I was going to reply to this to say that we rely on Paragon components to do the right thing re accessibility, but I see that this ProcessingNotification isn't actually using Paragon's Toast component!

So ya, this is a valid accessibility concern. Do you have time to convert this component to a proper Toast ?

Do you have time to convert this component to a proper Toast ?

@pomegranited Took some time but it is done now.

Please make the UX team aware of us. They originally asked us not to use Toast, because they wanted the same sort of animated notifications on the right-hand side of the screen that are used in the course experience. I do prefer the consistency of using Toast everywhere though.

@bradenmacdonald We can move the Toast popup to the bottom-right (for dir=ltr) and make sure it's positioned over the sidebar? Then we get the benefits of the Toast component and UX get their positioning.

@pomegranited Sounds good to me, please just confirm with @sdaitzman @jmakowski1123 . I'm not sure why we didn't do that before but I think the Paragon toasts can't be moved?

@bradenmacdonald Huh, really? I don't see why not, but I don't have context on this discussion either. Will see what @navinkarkera can do.

@pomegranited See #1211 and this comment. As long as you can satisfy those concerns, please definitely convert this to a standard toast :)

@bradenmacdonald @pomegranited Done. Moved standard toast over to right side and it has same animation as before and it automatically goes away after few seconds.

Also, if no onClose handler is passed (which is the case in course outline and units), the close icon is hidden at which point it looks same (a bit wider but that can be changed if require) as before.

Before:

After

@sdaitzman @jmakowski1123 Let me know if this looks good or if you want us to make some changes.

pomegranited · 2024-10-02T00:53:59Z

src/generic/processing-notification/index.jsx

+        <Button
+          variant="primary"
+          onClick={action.onClick}
+        >{action.label}
+        </Button>


The toast action button color looks too low contrast against the toast background:

I think if we can use a proper Toast component, this issue takes care of itself, e.g.

pomegranited · 2024-10-02T00:57:50Z

src/library-authoring/components/CollectionCard.test.tsx

+    expect(openMenuItem).toHaveAttribute('href', '/library/lb:org1:Demo_Course/collection/collection-display-name/');
+  });
+
+  it('should show confirmation box, delete collection and show toast to undo deletion', async () => {


Thank you for testing this feature so thoroughly!

navinkarkera · 2024-10-02T14:05:41Z

@pomegranited Thanks for the review! I have now converted ProcessingNotification to use Toast component from Paragon. We do see a warning from toast component due to passing html node as children instead of string which should be resolved by Rômulo's PR: openedx/paragon#3232

pomegranited

👍 Works perfectly @navinkarkera !

I tested this on my tutor dev stack
I read through the code and tests
I checked for accessibility issues by using my keyboard to navigate
Includes documentation
User-facing strings are extracted for translation

ChrisChV

@navinkarkera I added some small comments. Also, you need to fix conflicts

src/course-outline/CourseOutline.test.jsx

src/course-unit/CourseUnit.test.jsx

src/library-authoring/data/apiHooks.ts

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

navinkarkera force-pushed the navin/delete-collection branch from 2ebdce8 to 9f9e537 Compare September 27, 2024 13:18

navinkarkera commented Sep 27, 2024

View reviewed changes

src/generic/toast-context/index.tsx Outdated Show resolved Hide resolved

navinkarkera marked this pull request as ready for review September 27, 2024 13:45

navinkarkera requested a review from a team as a code owner September 27, 2024 13:45

navinkarkera force-pushed the navin/delete-collection branch 3 times, most recently from dabeaa8 to 5088ef4 Compare September 30, 2024 07:10

pomegranited reviewed Oct 2, 2024

View reviewed changes

navinkarkera force-pushed the navin/delete-collection branch 4 times, most recently from dbd4c1e to fe44d65 Compare October 2, 2024 13:40

navinkarkera changed the title ~~feat: delete collection~~ feat: delete collection [FC-0062] Oct 2, 2024

pomegranited approved these changes Oct 3, 2024

View reviewed changes

ChrisChV requested changes Oct 8, 2024

View reviewed changes

navinkarkera added 11 commits October 8, 2024 10:26

feat: delete collection

415c13b

refactor: add option to change delete modal variant

4abc7bb

fix: lint issues

b450d13

feat: update button status on delete

bf9e9fc

test: add tests for collection delete

3128590

fix: lint issues

c8dd4be

refactor: rebase upstream

ed6b0db

refactor: use toast component in processing notification

84bb5ce

refactor: improve css

b2ae41b

fix: collection sidebar behaviour on delete

910b465

fix: lint issues

a2f1cf1

navinkarkera added 7 commits October 8, 2024 10:32

fix: processing notification close button visibility

a74199d

test: fix processing notification tests

cd7cbf3

fix: tests

3adc3ea

fix: lint issues

6217089

refactor: move toasts to right side

5672b91

fix: resolve conflicts

71f735a

refactor: use screen and remove unnecessary args

316166b

navinkarkera force-pushed the navin/delete-collection branch from 5436424 to 316166b Compare October 8, 2024 05:17

ChrisChV approved these changes Oct 8, 2024

View reviewed changes

Merge branch 'master' into navin/delete-collection

26da705

ChrisChV enabled auto-merge (squash) October 8, 2024 16:49

ChrisChV merged commit 434fea3 into openedx:master Oct 8, 2024
6 checks passed

ChrisChV deleted the navin/delete-collection branch October 8, 2024 17:25

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Oct 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: delete collection [FC-0062] #1333

feat: delete collection [FC-0062] #1333

navinkarkera commented Sep 26, 2024 •

edited

Loading

openedx-webhooks commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

pomegranited left a comment

pomegranited Oct 2, 2024

navinkarkera Oct 2, 2024

bradenmacdonald Oct 2, 2024

pomegranited Oct 3, 2024

bradenmacdonald Oct 3, 2024

pomegranited Oct 3, 2024

bradenmacdonald Oct 3, 2024

navinkarkera Oct 3, 2024 •

edited

Loading

pomegranited Oct 2, 2024

pomegranited Oct 2, 2024

navinkarkera commented Oct 2, 2024

pomegranited left a comment

ChrisChV left a comment

feat: delete collection [FC-0062] #1333

feat: delete collection [FC-0062] #1333

Conversation

navinkarkera commented Sep 26, 2024 • edited Loading

Description

Supporting information

Testing instructions

Concerns

openedx-webhooks commented Sep 26, 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?

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navinkarkera Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navinkarkera commented Oct 2, 2024

pomegranited left a comment

Choose a reason for hiding this comment

ChrisChV left a comment

Choose a reason for hiding this comment

navinkarkera commented Sep 26, 2024 •

edited

Loading

openedx-webhooks commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

navinkarkera Oct 3, 2024 •

edited

Loading