[PB-5661] feat: favorite toggle from context menu and F shortcut - #2041
[PB-5661] feat: favorite toggle from context menu and F shortcut#2041victor-ferro wants to merge 10 commits into
Conversation
Deploying drive-web with
|
| Latest commit: |
5c64d18
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ee6916d3.drive-web.pages.dev |
| Branch Preview URL: | https://pb-5661-favorite-toggle.drive-web.pages.dev |
| }, | ||
| removeFavorites: (state: StorageState, action: PayloadAction<DriveItemData[]>) => { | ||
| state.favorites = state.favorites.filter( | ||
| (item) => !action.payload.find((i) => i.uuid === item.uuid && !!i.isFolder === !!item.isFolder), |
| @@ -0,0 +1,16 @@ | |||
| import { StorageTypes } from '@internxt/sdk/dist/drive'; | |||
There was a problem hiding this comment.
To ensure consistency with other files, the name should be ‘xxxx.service.ts’.
If we want to change it, we can discuss it, but ideally, even if it seems redundant, they should all have the same name so we don’t get confused
| useHotkeys( | ||
| 'f', | ||
| (e) => { | ||
| if (e.shiftKey) return; | ||
| e.preventDefault(); | ||
| if (!disableKeyboardShortcuts && selectedItems.length === 1) { | ||
| toggleFavorite(selectedItems); | ||
| } | ||
| }, | ||
| [disableKeyboardShortcuts, selectedItems, toggleFavorite], | ||
| ); |
There was a problem hiding this comment.
this hotkey function seems duplicated in DriveExplorer.tsx, maybe we should extract the hotkeys to a hook in order to implement the lib in one point and import in the other places
There was a problem hiding this comment.
Extracted the duplicated F shortcut into a shared useToggleFavoriteHotkey hook used by both DriveExplorer and SharedItemListContainer. Or were you referring to a hook covering all the useHotkeys we have across the project?
There was a problem hiding this comment.
No, not for all, for now, let’s stick to this one so we don’t stray from the scope of the task, and we’ll get round to the others later
| await Promise.all( | ||
| items.map(async (item) => { | ||
| const favorite = !item.isFavorite; | ||
|
|
||
| await setItemFavorite(item, favorite); |
There was a problem hiding this comment.
As several favorites can be sent at once, using Promise.all can mean that if one fails, we display as though everything has failed, when that is not the case. I think we should replace Promise.all with Promise.allSettled, so that the dispatches are applied only to those that were successful.
If they all fail, we could display a ‘rejected’ thunk (current toast), and if only some fail, perhaps display a ‘fulfilled’ thunk but with a different toast, such as ‘some favourites could not be updated’
There was a problem hiding this comment.
You're right. It's a use case that didn't affect this branch yet because we only allowed sending a single item to this function, but it's true that it's better to do it properly for the future. In fact, we already ran into this case in the PB-5662 branch. Done!
…tled and warn on partial failures
…leFavoriteHotkey hook
…precated StarIcon
# Conflicts: # src/views/Drive/components/DriveExplorer/DriveExplorer.tsx
|
2 participants
Description
Allows users to favorite/unfavorite files and folders from the item context menu, plus an
Fkeyboard shortcut.Fshortcut label right-aligned) in every single-item menu — right-click and three-dot — across Drive, Recents, Favorites and SharedFwith exactly one item selected toggles it; guarded against⇧F(new folder), open dialogs and TrashsetItemFavoriteservice +toggleFavoriteThunkpatch the item in place and keep the favorites listing in sync (patchItem,addFavorites/ newremoveFavoritesreducer); error toast on failureisFavoriteadded to drive/shared item types; translations in all localesRelated Issues
Relates to PB-5661
Related Pull Requests
Checklist
Testing Process
yarn vitest run src/views/Favorites src/app/store/slices/storage/storage.slice.test.ts src/views/Shared/hooks/useSharedContextMenu.test.ts— 6 files, 45 tests passing (against linked SDK)yarn check:i18nin syncFshortcut, immediate update in Drive and Favorites views, label/icon switching with item stateAdditional Notes
@phosphor-icons/reacthas no crossed-out star (StarSlashdoesn't exist), sosrc/assets/icons/star-slash.svgwas hand-made following Phosphor's stroke style (20×20,currentColor) and imported with?reactlike the existingmove.svg. If design wants a different one, swapping it is trivial — a single SVG referenced from one place.markFileAsFavorite/unmarkFileAsFavorite/markFolderAsFavorite/unmarkFolderAsFavoritecome from unreleased@internxt/sdkchanges (developed against a yarn-linked local SDK). CI will fail until that SDK version is published and bumped.