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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a69397f
feat: Allow click on searched file's path
paultranvan Nov 13, 2024
ffe3c0b
fix(search): Use MUI Link instead of `<a>` for search secondary title
Ldoppea Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/assistant/ResultMenu/ResultMenuContent.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ const SearchResult = () => {
<ResultMenuItem
key={result.id || idx}
icon={result.icon}
slug={result.slug}
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 😄

query={searchValue}
highlightQuery="true"
selected={
Expand Down
10 changes: 9 additions & 1 deletion src/assistant/ResultMenu/ResultMenuItem.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@ import ListItem from 'cozy-ui/transpiled/react/ListItem'
import ListItemIcon from 'cozy-ui/transpiled/react/ListItemIcon'
import ListItemText from 'cozy-ui/transpiled/react/ListItemText'
import SuggestionItemTextHighlighted from './SuggestionItemTextHighlighted'
import SuggestionItemTextSecondary from './SuggestionItemTextSecondary'

const ResultMenuItem = ({
icon,
primaryText,
secondaryText,
secondaryUrl,
slug,
selected,
onClick,
query,
Expand All @@ -32,7 +35,12 @@ const ResultMenuItem = ({
)

const secondary = highlightQuery ? (
<SuggestionItemTextHighlighted text={secondaryText} query={query} />
<SuggestionItemTextSecondary
text={secondaryText}
query={query}
slug={slug}
url={secondaryUrl}
/>
) : (
secondaryText
)
Expand Down
46 changes: 46 additions & 0 deletions src/assistant/ResultMenu/SuggestionItemTextSecondary.jsx
View file
Open in desktop
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 😉

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* Code copied and adapted from cozy-drive
*
* See source: https://github.com/cozy/cozy-drive/blob/fbe2df67199683b23a40f476ccdacb00ee027459/src/modules/search/components/SuggestionItemTextSecondary.jsx
*/
import React from 'react'

import AppLinker from 'cozy-ui/transpiled/react/AppLinker'
import SuggestionItemTextHighlighted from './SuggestionItemTextHighlighted'
import useBreakpoints from 'cozy-ui/transpiled/react/providers/Breakpoints'
import Link from 'cozy-ui/transpiled/react/Link'

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

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.

return (
<AppLinker app={app} href={url}>
{({ href, onClick }) => (
<Link
color="textSecondary"
underline="hover"
href={href}
onClick={e => {
e.stopPropagation()
if (typeof onClick == 'function') {
onClick(e)
}
}}
>
<SuggestionItemTextHighlighted
text={text}
query={query}
slug={slug}
/>
</Link>
)}
</AppLinker>
)
}

export default SuggestionItemTextSecondary
2 changes: 2 additions & 0 deletions src/assistant/Search/useFetchResult.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ export const useFetchResult = searchValue => {
return {
id: r.doc._id,
icon: icon,
slug: r.slug,
secondaryUrl: r.secondaryUrl,
primary: r.title,
secondary: r.subTitle,
onClick: () => {
Expand Down
Loading