-
Notifications
You must be signed in to change notification settings - Fork 110
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
A favorite icon will let the authenticated user add the resource to his favorites #214
Changes from 36 commits
39e4b7c
0e14896
a284a3c
213a635
42d8965
2cb0c3a
67fe726
efc1bcd
2e4ebe3
37f6ae8
e56da8b
d384237
8cfb1ca
19be961
7a646a3
0ff1b62
f328a01
7ae2c74
8381427
73330c2
2b7c30c
de22dc1
eac72ed
936768e
cb7dd17
1fdfb9b
f87b58b
57911d6
a78df16
e4439c3
e300f1b
92fee7e
cce793e
4697c3d
f6443b7
516d982
5c28cc5
b4f5762
3a3801c
b5dbca9
0e0f3ad
56f6592
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ import { | |
getUserName, | ||
getResourceTypesInfo | ||
} from '@js/utils/GNSearchUtils'; | ||
|
||
import debounce from 'lodash/debounce'; | ||
import CopyToClipboard from 'react-copy-to-clipboard'; | ||
import url from 'url'; | ||
import {TextEditable, ThumbnailEditable} from '@js/components/ContentsEditable/'; | ||
|
@@ -95,10 +95,12 @@ function DetailsPanel({ | |
editAbstract, | ||
editThumbnail, | ||
activeEditMode, | ||
closePanel | ||
closePanel, | ||
favourite, | ||
onFavourite, | ||
isLogged | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should use something more related to favourite instead of isLogged. isLogged is related to the user and the DetailsPanel component should not be aware of the user state maybe |
||
}) { | ||
|
||
|
||
const [editModeTitle, setEditModeTitle] = useState(false); | ||
const [editModeAbstract, setEditModeAbstract] = useState(false); | ||
|
||
|
@@ -133,6 +135,9 @@ function DetailsPanel({ | |
}, 700); | ||
}; | ||
|
||
const handleFavourite = () => { | ||
onFavourite(!favourite); | ||
}; | ||
|
||
const types = getTypesInfo(); | ||
const { | ||
|
@@ -234,6 +239,15 @@ function DetailsPanel({ | |
} | ||
{ | ||
<div className="gn-details-panel-tools"> | ||
{ | ||
isLogged && | ||
<Button | ||
variant="default" | ||
onClick={debounce(handleFavourite, 500)}> | ||
<FaIcon stylePrefix={ favourite ? `fa` : `far`} name="star" /> | ||
</Button> | ||
} | ||
|
||
{detailUrl && <OverlayTrigger | ||
placement="top" | ||
overlay={(props) => | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,12 +39,14 @@ import { | |
resourceLoading, | ||
setResource, | ||
resourceError, | ||
updateResourceProperties | ||
updateResourceProperties, | ||
SET_FAVOURITE_RESOURCE | ||
} from '@js/actions/gnresource'; | ||
import { | ||
getResourceByPk, | ||
createGeoStory, | ||
updateGeoStory | ||
updateGeoStory, | ||
setFavouriteResource | ||
} from '@js/api/geonode/v2'; | ||
import { parseDevHostname } from '@js/utils/APIUtils'; | ||
import uuid from 'uuid'; | ||
|
@@ -207,8 +209,33 @@ export const gnUpdateResource = (action$, store) => | |
.startWith(resourceLoading()); | ||
}); | ||
|
||
export const gnSaveFavouriteContent = (action$, store) => | ||
action$.ofType(SET_FAVOURITE_RESOURCE) | ||
.switchMap((action) => { | ||
const state = store.getState(); | ||
const pk = state?.gnresource?.data.pk; | ||
const favourite = action.favourite; | ||
const method = (favourite) ? 'post' : 'delete'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should have setFavouriteResource and deleteFavouriteResource There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably is better removeFavouriteResource instead of delete |
||
|
||
return Observable | ||
.defer(() => setFavouriteResource(pk, method)) | ||
.switchMap(() => { | ||
return Observable.of( | ||
updateResourceProperties({ | ||
'favourite': favourite | ||
}) | ||
); | ||
}) | ||
.catch((error) => { | ||
return Observable.of(resourceError(error.data || error.message)); | ||
}); | ||
|
||
}); | ||
|
||
|
||
export default { | ||
gnSaveContent, | ||
gnUpdateResource, | ||
gnSaveDirectContent | ||
gnSaveDirectContent, | ||
gnSaveFavouriteContent | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to pass the method?
I would prefer to have to separated function if they are needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to have one function, instead of 2 similar function, in this case change only the CRUD method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the aim is to use a single function I propose a different approach and do not expose the method.
Now we have here:
setFavouriteResource(pk, 'post')
// set favourite to truesetFavouriteResource(pk, 'delete')
// set favourite to falseMy proposal is to make this more explicit:
setFavouriteResource(pk, true)
// set favourite to truesetFavouriteResource(pk, false)
// set favourite to falseAnother proposal is to avoid as much as possible
eslint-disable
. We should try to follow the eslint rules: