Conversation
jairo-campos-JD
requested review from
RubenVerborgh,
james-martin-jd and
pablo-rodriguez-jd
| try { | ||
| if (webId) { | ||
| // We are fetching profile card document | ||
| const user = data[webId]; |
There was a problem hiding this comment.
Will webId always be the current user?
If yes, then why not data.user (and not pass in webId)?
If no, then perhaps calling it user is confusing.
There was a problem hiding this comment.
Yes it's a user I updated to data.user thank you !!!
RubenVerborgh
left a comment
RubenVerborgh
left a comment
There was a problem hiding this comment.
Looks good, but consider paying more attention to simplifying code: there are several repeated patterns that take many lines, for which you can create or reuse an existing helper.
| toastManager.add (['Error', error.message], { | ||
| appearance: 'error', | ||
| }); | ||
| } |
There was a problem hiding this comment.
You should be able to write the above much more easily as follows:
const image = useLDflexValue(`[${webId}].image`);
const photo = useLDflexValue(`[${webId}].vcard$hasPhoto`);
const src = image || photo;| const {user} = data; | ||
| image | ||
| ? await user.image.set (uri) | ||
| : await user.image.add (uri); |
There was a problem hiding this comment.
Yes you right, done.
| toastManager.add (['Error', error.message], { | ||
| appearance: 'error', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Your toastManager code takes up a lot of lines.
Consider creating shortcuts like toastManager.success and toastManager.error.
There was a problem hiding this comment.
You can even avoid the catch if you write a helper like
toastManager.report(async () => {
}, "Profile image update");
with
toastManager.report = async function(action, successMsg) {
try { await action(); }
catch (error) { toastManager.error(error); return; }
toastManager.success(successMsg);
}
There was a problem hiding this comment.
We want to remove this toastsManager and create a new one to avoid the use of consumer in all places, so I don't know if we need to do this refactor since we will remove it.
3 participants
hooks, websockets updates, This will need to update @inrupt/solid-react-component package. Please do not merge it yet.