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

Add informations block #2626

Merged
merged 5 commits into from
Nov 26, 2024
Merged

Add informations block #2626

merged 5 commits into from
Nov 26, 2024

Conversation

JF-Cozy
Copy link
Contributor

@JF-Cozy JF-Cozy commented Nov 25, 2024

En plus des bloc connecteur et qualification, on ajoute un nouveau bloc d'information. On déplace également la partie qualification dans ce nouveau bloc.

@JF-Cozy JF-Cozy requested a review from acezard as a code owner November 25, 2024 14:32
* @param {number} bytes - file bytes
* @returns {string}
*/
export const makeSize = bytes => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is already a similar function word done inside cozy-client models instances maybe should harmonize this. Especially if it means we can remove the dependency on filesize in Drive later on

Copy link
Contributor Author

@JF-Cozy JF-Cozy Nov 25, 2024

Choose a reason for hiding this comment

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

je n'ai pas trouvé côté cozy-client, je suis parti sur du custom du coup. Après vérification dans Drive, on utilise une lib filesize https://www.npmjs.com/package/filesize pour ça. Comme ça tient en 4 lignes ici, pas sûr que la lib se justifie 🤔

Copy link
Contributor

@cballevre cballevre Nov 25, 2024

Choose a reason for hiding this comment

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

My point was to make it shared via cozy-client rather than adding a custom one. You don't have all your functions ready-made, but some of them overlap with the model/intance functions (example of code). We could divide yours into several helpers so as not to repeat the same type of work. I agree for 4 line the lib is not justified in cozy-drive.

What's more, you'll save time when you want to migrate the FolderPicker inside cozy-ui because it needs this function. Now, its done with the lib filesize (code) 🙂

Copy link
Contributor Author

@JF-Cozy JF-Cozy Nov 26, 2024

Choose a reason for hiding this comment

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

Ok je verrai à ce moment là. Si on doit extraire le FolderPicker (typiquement à ce stade je préfère concentrer mon énergie sur ce sujet de FolderPicker plutôt que cette question de mutualisation) et qu'il y a besoin de mutualiser cette méthode, je le ferai à ce moment là.

Le chantier du nouveau viewer est conséquent, je pense que c'est bien de noter ces points de refacto afin d'améliorer la base de code et de mutualiser autant que possible, mais on navigue plutôt à vue sur cette feature qu'on souhaite sortir au fil de l'eau et tester tôt. A mon avis ce genre de retour sont plutôt à traiter dans un deuxième temps, profiter d'une phase de recette par exemple pour faire la refacto en parallèle, tout en notant ces retours dans le carte linear afin de ne pas les oublier.

packages/cozy-viewer/src/Panel/helpers.js Show resolved Hide resolved
@JF-Cozy JF-Cozy merged commit a696472 into master Nov 26, 2024
2 checks passed
@JF-Cozy JF-Cozy deleted the feat/viewer/VER-1094 branch November 26, 2024 07:44
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