Skip to content
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: Avoid close Tags drawer on edit #34

Closed

Conversation

ChrisChV
Copy link
Member

@ChrisChV ChrisChV commented Apr 17, 2024

Description

  • To avoid lose user data
  • Wrap with ContentTagsDrawerSheet to build drawer on MFE

Supporting information

The footer of the drawer animates to draw the user's attention to the actions (see Figma). Perhaps like Blink 2 at this link

Testing instructions

Copy link

@yusuf-musleh yusuf-musleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisChV Great work on this, the code looks good to me and it works well!

However I am still able to close the drawer when pressing escape even when there are unsaved changes. It might be worth adding some logic to the keypress handler to prevent it from closing when there are unsaved changes.

@ChrisChV
Copy link
Member Author

@yusuf-musleh Thanks for the review!

However I am still able to close the drawer when pressing escape even when there are unsaved changes. It might be worth adding some logic to the keypress handler to prevent it from closing when there are unsaved changes.

Thanks for catch that! I fixed that here: f4d7e04

Copy link

@yusuf-musleh yusuf-musleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 @ChrisChV Great work! Thanks for addressing the issue, its working as expected. For the codecov issue that is failing in the CI, sometimes it gets fixed when re-triggering the CI to run again, I usually do that by doing git commit --amend --noedit and then force pushing, it makes no changes to the code, just changes the commit hash because we are amending (nothing) to it.

  • I tested this: Followed testing instructions
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@ChrisChV ChrisChV force-pushed the chris/FAL-3711-close-drawer branch from f4d7e04 to dc44778 Compare April 22, 2024 15:23
@ChrisChV ChrisChV force-pushed the chris/FAL-3645-refined-tag-drawer branch from 2e5c81f to cdaf7b5 Compare April 22, 2024 15:38
@ChrisChV ChrisChV force-pushed the chris/FAL-3711-close-drawer branch 4 times, most recently from 44b40ed to 9483bac Compare April 23, 2024 14:33
@ChrisChV ChrisChV force-pushed the chris/FAL-3645-refined-tag-drawer branch from b491d9b to 8110861 Compare April 25, 2024 19:12
yusuf-musleh and others added 3 commits April 26, 2024 00:42
It contains different changes to achieve the reading and editing mode of the drawer tag:

* Manage tags drawer footer with buttons added.
* Creation of ContentTagsDrawerContext.
* Creation of global state and global removed state to allow edit mode.
* Update API client to match with openedx-learning 0.9.1: Save tags of multiple taxonomies; to save all tags added/removed on edit mode
* Extract TagsTree and use it on the Tags Drawer.
* Update TagsTree to allow edit mode.
* Add a Toast on Tags Drawer; show the toast afert save.
* Scrolling + sticky footer on tags drawer
* Avoid close Tags drawer on edit
    * To avoid lose user data
    * Wrap with ContentTagsDrawerSheet to build drawer on MFE
* ContentTagsDrawerSheetContext created
* Not close drawer with Escape is pressed when container is blocked
@ChrisChV ChrisChV force-pushed the chris/FAL-3711-close-drawer branch from d36bf33 to 73c978b Compare April 25, 2024 20:05
@ChrisChV
Copy link
Member Author

Close in favor of openedx#965

@ChrisChV ChrisChV closed this Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants