feat: Drag-and-drop entire collections #1056

jcbcapps · 2023-06-22T21:33:25Z

SC-651

Proposed changes

Add drag-and-drop for collections
Move most component logic from MySpace component into a new MySpaceContext

To do:

~~Add test for MySpaceContext~~

Reviewer notes

Run the app as usual and test the following:

Drag-and-drop using a mouse
Drag-and-drop using a keyboard (tab to focus, space/enter to pick up/drop collections, arrow keys to move)
Here is the e2e test to review: https://github.com/USSF-ORBIT/ussf-portal/pull/295

Setup

Start the system

yarn services:up
yarn dev
cd ../ussf-portal-cms
yarn dev

Login to the portal http://localhost:3000

Start storybook

yarn storybook

Login to storybook http://localhost:6006, though the command above should open it for you

Code review steps

As the original developer, I have

Met the acceptance criteria
Created new stories in Storybook if applicable
Created/modified automated unit tests in Jest
Created/modified automated E2E tests
Followed guidelines for zero-downtime deploys, if applicable
Use ANDI to check for basic a11y issues

As a reviewer, I have

Check out our How to review a pull request document.

Screenshots

shortcut-integration · 2023-06-22T21:33:27Z

This pull request has been linked to Shortcut Story #651: Drag and drop entire collections.

src/components/CustomCollection/EditableCollectionTitle.tsx

jcbcapps · 2023-06-27T14:14:14Z

src/types/index.ts

+  // Added this because the bookmarks field is required on the Collection interface below, but
+  // previously did not exist on the Widget type, which was causing a type error when trying to
+  // map over the user's MySpace to render the widgets.
+  bookmarks?: MongoBookmark[]
  // default indicates if the widget is automatically added for the user
  default?: boolean
+  // id is for dnd-kit
+  id?: string


As the comment says, the difference in the presence of the bookmarks field was causing issues in a union type down the line. I believe we discussed this a few months back and a potential refactor of the MySpace-related types, but can't remember the decision. I added both bookmarks and id as optional fields, but let me know if that is potentially problematic and I'd be happy to take another look/create a new type.

Is there a way we could use _id as the id field for dnd-kit? Or does it have to be id?

I remember why I did this now and not sure it would be best explained here. Maybe we can have a chat at team sync about it?

jcbcapps · 2023-06-27T19:00:09Z

src/components/MySpace/MySpace.tsx

+              // Ignoring the following line because an id field has already been added to the Widget type to accomodate
+              // the unique identifier necessary for @dnd-kit.
+              // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+              // @ts-ignore
+              items={mySpace}


I opted to ignore this type for a few reasons. First, I already added an optional id field to the Widget type to accommodate the unique identifier. Second, the type error that shows here is complaining about the MySpace type and how it is not compatible with something like (UniqueIdentifier | {id: UniqeIdentifier | undefined})[] and I could not get that to line up with our current MySpace type. Third, even if I could get the types to line up, the UniqueIdentifier field would have to be required, and my initial thought it that we don't want that for MySpace. Maybe I'm missing something? Not sure. If you have any ideas/solutions, let me know!

jcbcapps · 2023-06-29T20:18:54Z

src/components/ApplicationsTable/ApplicationsTable.tsx

-  userCollectionOptions = [],
+  userCollectionOptions,
  handleAddToCollection,
-  canAddNewCollection = true,


I was hunting for ways to bump test coverage and stumbled upon this. I removed these default params because they were never being evaluated due to the parent component having the same props with the same default param values.

jcbcapps · 2023-06-29T21:53:28Z

src/components/CustomCollection/CustomCollection.tsx

-      handleAddBookmark(
-        existingLink.url || '',
-        existingLink.label,
-        existingLink.id
-      )
+      handleAddBookmark(existingLink.url, existingLink.label, existingLink.id)


I changed this because url is a required value on the CMSBookmark type, so existingLink.url should never be false/empty

gidjin · 2023-07-06T06:45:00Z

src/stores/myspaceContext.tsx

+    return
+  },
+  isCollection: /* istanbul ignore next */ () => {
+    return true || false


Question: do these need to return true || false for the type system?

gidjin · 2023-07-06T06:52:58Z

src/types/index.ts

+  // Added this because the bookmarks field is required on the Collection interface below, but
+  // previously did not exist on the Widget type, which was causing a type error when trying to
+  // map over the user's MySpace to render the widgets.
+  bookmarks?: MongoBookmark[]
  // default indicates if the widget is automatically added for the user
  default?: boolean
+  // id is for dnd-kit
+  id?: string


Is there a way we could use _id as the id field for dnd-kit? Or does it have to be id?

jcbcapps · 2023-07-06T14:21:10Z

Is there a way we could use _id as the id field for dnd-kit? Or does it have to be id?

@gidjin I think you are right about this. The decision to do this may have just been a result of me working on this for a while and my brain burning out a little bit. I'm going to take another look at it.

shkeating

looks good to me, and passed all applicable trusted tester criteria. good work :) my only comment is that this is the kind of PR where adding a gif of the behavior to the screenshots section is helpful, but i went ahead and added it.

shkeating · 2023-07-06T20:44:36Z

is changing the contents of the myspace example in storybook on purpose?

jcbcapps · 2023-07-06T20:52:34Z

is changing the contents of the myspace example in storybook on purpose?

@shkeating Nope. Looks like I forgot to mock the new myspaceContext. Good catch

…cles for cms users (#1057) * fix: announcement with CTA to deleted article links to 404 * fix: return 404 if article not found and cms user

* Signing WIP commit * Save point for version that compiles. Some missing styles but looking good * fix a couple compile errors * add back the postinstall we need to copy vendor files * case sensitivity in test somehow got out of sync * fix ordering of stylesheets to correct discrepancies * cleaning up wip files * more cleanup for consistency * remove unused var theme-hero-image * chore(deps): update storybook monorepo to v7 * fix storybook upgrade * update storybook again * disable storybook auto doc gen * fix strange error in modalContext tests * split storybook build from happo build * upgrade to latest storybook * use shortened path for index.scss file * update comment * moved styles that resolve duplicate alert icon issue into storybook only stylesheet * adjusts css to work on storybook and in production to hide random extra icon * adds slim attribute to stories for alerts to match what they used to look like * remove deprecated file * add missing import statements * workaround for prefixing css in storybook * putting back initIcons because we need those! * remove unneccesary comments --------- Co-authored-by: John Gedeon <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Shauna Keating <[email protected]>

…1055) * adds open in new window text to feautured shortcuts * adds opens in a new window note to orbit link in footer * adds open in a new window tag to footer links * adds note to screen reader users about functionality of color theme switcher * adds unique button names for each bookmark in collection * refectors widget headers and widget settings buttons to give each button a unique settings button label that tells the user what widget its for * adds context to custom collections controls about what custom collection would be edited or deleted for screenreader users * labels remove recent news button more specifically * adds link name to add to myspace option for screenreader users * updates Add to My Space strings to include bookmark label * adds data test id to add to myspace button to help the test find it correctly with using the text inside the button no longer being an option * adds data test id to add to myspace button to help the test find it correctly with using the text inside the button no longer being an option * updates featured shortcut test and guardian ideal test to reflect name changes on featured shortcut item and widget settings * undo test changes that did not work * removes weird space * removes data test id that didnt work * moves h3 tag to correct location * adjusts Guardian Ideals header to match its test query * makes adjustments to recent news widget test to reflect wdiget component changes * updates Featured Shortcuts test file * makes changes to Applications Table test file * refactors Removable Bookmarks test to account for link name changes * adjusts implementation of h3 on widget components * fixes most of sites & applications tests * adjusts Custom Collection tests * adjusts widget back so it doesnt break a ton of tests * bookmark should take a label prop * pass in label to bookmark * fix various tests * fix: guardian ideal is singular --------- Co-authored-by: John Gedeon <[email protected]>

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: John Gedeon <[email protected]> ## [4.22.0](v4.21.0...v4.22.0) (2023-07-10) ### Features * a11y: assigns unique page titles to each page in the portal ([#1037](#1037)) ([7b21d50](7b21d50)) * a11y: creates alternative means to access inaccessible information in latest announcements carousel ([#1032](#1032)) ([3f9a2ff](3f9a2ff)) * Drag-and-drop entire collections ([#1056](#1056)) ([f415265](f415265)) * Filter search results client ([#1027](#1027)) ([35f6f90](35f6f90)) ### Bug Fixes * a11y: adjusts heading order on myspace page ([#1034](#1034)) ([4c6d1aa](4c6d1aa)) * a11y: adjusts link names to improve usability on assistive tech ([#1055](#1055)) ([41854ca](41854ca)) * **deps:** update dependencies ([#935](#935)) ([c15e83d](c15e83d)) * handle removed articles linked in announcements and missing articles for cms users ([#1057](#1057)) ([e9a2459](e9a2459)) * Migration to remove duplicate users ([#1040](#1040)) ([eec8564](eec8564))

jcbcapps commented Jun 23, 2023

View reviewed changes

src/components/CustomCollection/EditableCollectionTitle.tsx Outdated Show resolved Hide resolved

jcbcapps commented Jun 27, 2023

View reviewed changes

jcbcapps marked this pull request as ready for review June 27, 2023 20:13

jcbcapps requested a review from a team as a code owner June 27, 2023 20:13

jcbcapps commented Jun 29, 2023

View reviewed changes

jcbcapps requested a review from a team as a code owner June 30, 2023 14:57

gidjin approved these changes Jul 6, 2023

View reviewed changes

shkeating approved these changes Jul 6, 2023

View reviewed changes

jcbcapps and others added 16 commits July 7, 2023 17:11

Add basic dndkit implementation

5262251

Add sorting to drag and drop

4cf338b

Sort visibleBookmarks to match sorted dnd items array

beaeb2d

Fix removed bookmarks breaking drag and drop

7b95e76

Update comment

0fcfd07

Add CustomBookmark

3962181

Remove unused activeId and fix Generic Object Injection Sink

288a1ed

add type to function

f3e3a79

update ui when add/remove and save on order change

9ccd9a2

Update tests

0bb6e5b

Update visibleBookmarks and removedBookmarks when bookmarks prop changes

0e196fb

Fix import

11cf0b5

Update test

e6749b5

Memoize bookmarks

7ce0133

Fix flicker

4454543

Add mutation

c0e2f42

jcbcapps and others added 23 commits July 7, 2023 17:21

Remove console.log statements

5a5fcb3

Add test file for myspaceContext

9f37b9f

Add test

5dbb5d4

Add tests

5946b2c

Add test

350bef7

Add test to check for light theme

007e143

Update tests and make AddWidget consume context

6e06763

Ignore mock functions for test coverage

adfced2

Change it to test

a7d040a

Update tests and remove unnecessary default params

0eea90d

Update test to check collection without title

8f33911

Add test to check for no username

f013e5d

Add test to check for an image

63d1c4d

Add test for Featured Shortcuts

6d6eff7

Update test and make props required

a56f56e

Remove empty string

2530f3d

Update storybook

11f08ed

fix: handle removed articles linked in announcements and missing arti…

5f276c0

…cles for cms users (#1057) * fix: announcement with CTA to deleted article links to 404 * fix: return 404 if article not found and cms user

Update to use mock myspace context

2522e39

Add comment and cleanup

7a16aef

fixing rebase whoops

5d3c386

abbyoung force-pushed the sc-651-drag-entire-collections branch from 3c16af0 to 5d3c386 Compare July 10, 2023 17:26

merge in main

acedd76

jcbcapps merged commit f415265 into main Jul 10, 2023

jcbcapps deleted the sc-651-drag-entire-collections branch July 10, 2023 18:31

github-actions bot mentioned this pull request Jul 10, 2023

chore(main): release 4.22.0 #1039

Merged

jcbcapps mentioned this pull request Jul 13, 2023

fix: Fix drag-and-drop bug #1066

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Drag-and-drop entire collections #1056

feat: Drag-and-drop entire collections #1056

jcbcapps commented Jun 22, 2023 •

edited by shkeating

Loading

shortcut-integration bot commented Jun 22, 2023

jcbcapps Jun 27, 2023

gidjin Jul 6, 2023

jcbcapps Jul 6, 2023

jcbcapps Jun 27, 2023 •

edited

Loading

jcbcapps Jun 29, 2023

jcbcapps Jun 29, 2023

gidjin Jul 6, 2023

jcbcapps Jul 6, 2023

gidjin Jul 6, 2023

jcbcapps commented Jul 6, 2023

shkeating left a comment

shkeating commented Jul 6, 2023 •

edited

Loading

jcbcapps commented Jul 6, 2023

feat: Drag-and-drop entire collections #1056

feat: Drag-and-drop entire collections #1056

Conversation

jcbcapps commented Jun 22, 2023 • edited by shkeating Loading

SC-651

Proposed changes

Reviewer notes

Setup

Start the system

Start storybook

Code review steps

As the original developer, I have

As a reviewer, I have

Screenshots

shortcut-integration bot commented Jun 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcbcapps Jun 27, 2023 • edited Loading

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

jcbcapps commented Jul 6, 2023

shkeating left a comment

Choose a reason for hiding this comment

shkeating commented Jul 6, 2023 • edited Loading

jcbcapps commented Jul 6, 2023

jcbcapps commented Jun 22, 2023 •

edited by shkeating

Loading

jcbcapps Jun 27, 2023 •

edited

Loading

shkeating commented Jul 6, 2023 •

edited

Loading