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

[PRD-610] feat: Allow click on searched file's path #2246

Merged
merged 2 commits into from
Nov 13, 2024
Merged

[PRD-610] feat: Allow click on searched file's path #2246

merged 2 commits into from
Nov 13, 2024

Conversation

paultranvan
Copy link
Contributor

@paultranvan paultranvan commented Nov 13, 2024 •
edited
Loading

See also cozy/cozy-libs#2615

### ✨ Features

* [feat: Allow click on searched file's path](https://github.com/cozy/cozy-home/commit/a69397f6e77675a3c837f2dcfbd835e0eaec0afe)

### 🐛 Bug Fixes

*

### 🔧 Tech

*

paultranvan and others added 2 commits November 13, 2024 16:21
@paultranvan
feat: Allow click on searched file's path
a69397f 
In the search, we display the folder path for files results.
It is not possible to click on it, to directly open the folder rather
than the file itself.
@Ldoppea @paultranvan
fix(search): Use MUI Link instead of <a> for search secondary title
ffe3c0b
@bundlemon BundleMon
Copy link

bundlemon bot commented Nov 13, 2024

BundleMon

Files updated (1)
Status Path Size Limits
✅ app/home.(hash).js
 56.19KB (+150B +0.26%) -
Unchanged files (12)
Status Path Size Limits
✅ vendors/home.(hash).js
 1.57MB -
✅ services/softDeleteOrRestoreAccounts/home.js
 466.04KB -
✅ services/updateAccounts/home.js
 463.17KB -
✅ services/deleteAccounts/home.js
 311.82KB -
✅ services/myselfFromIdenties/home.js
 236.25KB -
✅ services/polyfillFetch/home.js
 97.38KB -
✅ vendors-home.(hash).(hash).min.css
 37.78KB -
✅ intents/home.(hash).js
 26.57KB -
✅ services/attributesHelpers/home.js
 15.22KB -
✅ app-home.(hash).min.css
 2.21KB -
✅ intents/index.html
 652B -
✅ intents-home.(hash).min.css
 158B -

Total files change +150B 0%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

@paultranvan paultranvan requested review from Ldoppea and JF-Cozy and removed request for Ldoppea November 13, 2024 15:30
JF-Cozy
JF-Cozy approved these changes Nov 13, 2024
View reviewed changes
src/assistant/ResultMenu/ResultMenuContent.jsx
primaryText={result.primary}
secondaryText={result.secondary}
secondaryUrl={result.secondaryUrl}
Copy link
Contributor

@JF-Cozy JF-Cozy Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

est-ce qu'on a besoin de rajouter ces 2 props qui ne sont au final utilisé que par SuggestionItemTextSecondary ? 🤔 Est-ce qu'on peut pas plutôt générer le lien en amont et transmettre un composant via secondaryText ? Car au final ici on ne fait qu'afficher autre chose en secondaryText selon certaines conditions...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

l'API de search ne peut pas transmettre un composant, ça va passer par le dataproxy et de la sérialisation. Et ça me semble bien qu'elle n'ait aucune responsabilité UI

Copy link
Contributor

Choose a reason for hiding this comment

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

j'ai pas compris le rapport avec l'API. Ici je parle des composants front qu'on a construit dans l'app. Ce qu'on fait ici c'est que dans un certain cas (quand y'a une url) on veut afficher un lien plutôt qu'un texte, et ce au final dans un ListItemText en prop "secondary". Au lieu de passer result.secondaryUrl je me demandais si on ne pouvait pas rester avec la prop secondaryText, sauf qu'on ne lui passerait pas result.secondary mais un composant (celui nécessaire pour construire le lien)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah je pensais que tu parlais côté dataproxy. Pas d'avis particulier, si ce n'est que ça marche comme ça et n'apporte pas de désavantage particulier 😄

src/assistant/ResultMenu/SuggestionItemTextSecondary.jsx
Copy link
Contributor

Choose a reason for hiding this comment

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

on peut squash les 2 commits je pense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai voulu laisser la contribution de @Ldoppea 😉

Ldoppea
Ldoppea approved these changes Nov 13, 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.

My understanding is that this PR and cozy/cozy-libs#2615 are both retro-compatible as we only add new attributes and in cozy-home we fallback to previous behaviour when url is not set. Is that correct? If yes feel free to merge it.

src/assistant/ResultMenu/SuggestionItemTextSecondary.jsx
return <SuggestionItemTextHighlighted text={text} query={query} />
}

const app = { slug }
Copy link
Member

Choose a reason for hiding this comment

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

Note: For now it is not a problem, but <AppLink> may need other props like app.mobile to correctly handle some links. This prop is used only by the flagship app and to open cozy-pass links, so we are safe for now.

src/assistant/ResultMenu/SuggestionItemTextSecondary.jsx
const SuggestionItemTextSecondary = ({ text, query, url, slug }) => {
const { isMobile } = useBreakpoints()

if (isMobile || !url) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we secure this by checking also the value of slug in addition to url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, because I tested with an undefined slug, and it still works 🤷
This does not look mandatory, but AppLinker complains if it is missing, so I kept it

@paultranvan
Copy link
Contributor Author

My understanding is that this PR and cozy/cozy-libs#2615 are both retro-compatible

Your understanding is correct 👍

@paultranvan paultranvan merged commit 914c604 into master Nov 13, 2024
4 checks passed
@paultranvan paultranvan deleted the with-search-path-click branch November 13, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JF-Cozy JF-Cozy JF-Cozy approved these changes

@Ldoppea Ldoppea Ldoppea approved these changes

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

@doubleface doubleface Awaiting requested review from doubleface doubleface 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.
3 participants
@paultranvan @Ldoppea @JF-Cozy