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.

Sign up for GitHub

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

Jump to bottom

[VO-847] feat: Add favorite to files #3184

Merged
merged 11 commits into from
Jul 11, 2024
Merged

[VO-847] feat: Add favorite to files #3184

merged 11 commits into from
Jul 11, 2024

Conversation

cballevre
Copy link
Member

@cballevre cballevre commented Jul 9, 2024 •
edited
Loading 

### ✨ Features

* Save files into favorite with flag `drive.show-favorites-dev`

### 🔧 Tech

*  Update cozy-ui from 110.1.0 to 110.6.0

Remaining work :

  • make types work

Related PRs :

@bundlemon BundleMon
Copy link

bundlemon bot commented Jul 9, 2024 •
edited
Loading

BundleMon

Files updated (4)
Status Path Size Limits
✅ app/drive.(hash).js
 176KB (+1.88KB +1.08%) 300KB
✅ public/drive.(hash).js
 1.56MB (+1.69KB +0.11%) 1.7MB
✅ vendors/drive.(hash).js
 1.78MB (+1.27KB +0.07%) 2MB
✅ intents/drive.(hash).js
 105.99KB (+249B +0.23%) 190KB
Unchanged files (14)
Status Path Size Limits
✅ public/pdf.worker.entry.(hash).worker.js
 345.35KB 350KB
✅ services/qualificationMigration/drive.js
 279.88KB 500KB
✅ services/dacc/drive.js
 260.92KB 500KB
✅ public/cozy-client-js.js
 159.28KB 160KB
✅ public/drive.(hash).min.css
 69.71KB 100KB
✅ app-drive.(hash).min.css
 37.84KB 56KB
✅ intents-drive.(hash).min.css
 33.01KB 40KB
✅ onlyOffice/slide.pptx
 24.83KB 25KB
✅ onlyOffice/text.docx
 5.85KB 6KB
✅ onlyOffice/spreadsheet.xlsx
 5.02KB 6KB
✅ manifest.webapp
 1.87KB 2KB
✅ index.html
 596B 1KB
✅ intents/index.html
 507B 1KB
✅ manifest.json
 185B 1KB

Total files change +5.07KB +0.1%

Groups updated (6)
Status Path Size Limits
✅ img/**
 5.85KB (+2.44KB +71.6%) -
✅ app/**
 176KB (+1.88KB +1.08%) -
✅ public/**
 2.13MB (+1.69KB +0.08%) -
✅ vendors/**
 1.78MB (+1.27KB +0.07%) -
✅ intents/**
 106.49KB (+249B +0.23%) -
✅ services/**
 540.8KB (-11B 0%) -
Unchanged groups (1)
Status Path Size Limits
✅ onlyOffice/**
 35.7KB -

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

@cballevre cballevre force-pushed the feat/add-favorite branch 3 times, most recently from d210ddc to 6fa1e2e Compare July 11, 2024 08:55
@cballevre cballevre marked this pull request as ready for review July 11, 2024 08:55
@cballevre cballevre requested a review from acezard as a code owner July 11, 2024 08:55
@cballevre cballevre requested a review from Ldoppea July 11, 2024 08:55
Ldoppea
Ldoppea approved these changes Jul 11, 2024
View reviewed changes
Copy link
Member

@Ldoppea Ldoppea left a comment

Choose a reason for hiding this comment

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

Only mandatory change is the typo on translations, everything else is nit.

Also another nit on the refactor: Use IOCozyFile type from cozy-client commit message: Is that related to cozy/cozy-client#1496 ? If yes, I like when those relations are specified in the commit message, so it is easy to navigate between PRs and understand why things are done and where they are used (but this is purely nit)

src/locales/en.json Outdated Show resolved Hide resolved
src/modules/navigation/FavoriteItems.tsx

return (
<NavItem key={file._id}>
{/* eslint-disable-next-line @typescript-eslint/no-misused-promises */}
Copy link
Member

Choose a reason for hiding this comment

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

cc @acezard you created a method to wrap promises and avoid this ESLint error, do you remember its name? I hade safePromise in mind, but it seems not to be used in react components, so maybe it is another method?

Copy link
Contributor

Choose a reason for hiding this comment

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

it is, but it's only in the react-native project, not exported

src/modules/views/Favorites/FavoritesView.tsx
Comment on lines +77 to +79
) as {
data?: IOCozyFile[] | null
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would suggest avoiding inline types and instead declare them somewhere else and so you can write as SomeTypeName instead of as { ...... }

This would ease code readability as inline types are often the main reason people find TS hard to read (because you had a block of {} only for typing where people expect to get logic code)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well noted, I'll try to improve that in my next PR 👍

src/modules/views/Recent/index.jsx
@@ -17,13 +18,14 @@ import {
download,
trash,
rename,
duplicate,
Copy link
Member

Choose a reason for hiding this comment

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

In multiple places you remove duplicate but commit messages only talk about adding favorites concept. Is that expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's deliberate, but you're right to point it out - I should have separated it out in a separate commit. It's done with an explanation now 🙂

@cballevre
feat: Update cozy-ui from 110.1.0 to 110.6.0
52eb250
@cballevre
feat: Update cozy-client from 48.6.0 to 48.7.0
93723d8
@cballevre
feat: Update cozy-flags from 3.2.2 to 4.0.0
348cb14
@cballevre
refactor(ModalContext): Convert into typescript
f022915
@cballevre
refactor(NavContent): Convert into Typescript
f04806e
@cballevre
feat: Add favorite actions
65a9745
@cballevre
feat: Show favorites into navigation
6454d2d
@cballevre
feat(Recent): Remove duplicate from files action
58b43d6 
With the design team we decided to align the experience of the actions on a file for the Favorites, Recent, and Sharing pages. That's why I removed Duplicate because it could cause weird behaviour when you're at the end of the list.
@cballevre
refactor: Use IOCozyFile type from cozy-client
03d3156 
I transferred the fields that was missing in IOCozyFile into cozy-client in this PR : cozy/cozy-client#1496
@cballevre
fix: Remove isExternal as it is no longer used
27d9b5c 
This property was used when we were displaying shared drives files as secondary nav item
@cballevre
fix(FolderPickerTopbar): Remove unnecessary type cast
048a737 
This was used because JSDoc type cast doesn't work well with VSCode as the error disappear I chose to remove it
@cballevre cballevre merged commit 7e048ce into master Jul 11, 2024
3 checks passed
@cballevre cballevre deleted the feat/add-favorite branch July 11, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ldoppea Ldoppea Ldoppea approved these changes

@acezard acezard Awaiting requested review from acezard acezard is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cballevre @Ldoppea @acezard