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

implemneted feature 'add me to your friends' https://github.com/solid/profile-pane/issues/8 #15

Merged
merged 8 commits into from
Oct 7, 2021

Conversation

timea-solid
Copy link
Member

Added the new 'Add me to your friends' button under profile (see screenshot).
image
The test case with logged in user will change when auth changes.

@bourgeoa
Copy link
Contributor

@theRealImy I shall try it tomorrow
2 questions on the code. I'm a non professional :

  • why possitive with double s
  • rdflib is called either directly or through UI. These may differ version. It is a general point in solidos

@timea-solid
Copy link
Member Author

I improved the code as suggested.

@theRealImy I shall try it tomorrow
2 questions on the code. I'm a non professional :

* why possitive with double s

* rdflib is called either directly or through UI. These may differ version. It is a general point in solidos

@bourgeoa
Copy link
Contributor

There seem to be 2 issues partly related :

  • the addMe button should be Greyed when not usable : no new friend to add (already exist or not logged in)
  • you should not be able to add yourself to the friend's list

@timea-solid
Copy link
Member Author

timea-solid commented Sep 2, 2021

I've improved the code based on the feedback.
However to keep UI consistent with the 'Chat with me' button which is also displayed on the profile pane, i did not add a greyed out button but followed same design like 'Chat with me'. I propose revising button design afterwards, for all buttons.
For now:

  • if one is not logged in the button is like the 'chat with me' without filling

Screenshot 2021-09-01 at 14 47 04

  • the button is active and if pressed one gets the same error message like on Chat with me:

Screenshot 2021-09-02 at 11 33 01

  • if one is logged in and friend does not exist, the button gets a filling like:

Screenshot 2021-09-01 at 14 46 43

  • if friend is added a message appears:

Screenshot 2021-09-02 at 11 34 29

  • if I log in and I already have this friend in my list I see a different message on the button:

Screenshot 2021-09-01 at 14 45 30

  • If I still press the button even if 'Already part of friend list' I get a message like:

Screenshot 2021-09-01 at 14 46 01

Part missing which I am not sure if it should work: button design is not refreshed after clicking it, should the UI have this behaviour? Also, I noticed that the data is not really updated real-time.
Example:

  • I add a new friend by using the new button
  • I go to my pod and delete the friend again (manually)
  • I try to add the friend again (but I did not refresh the page yet)
  • friend is not added. -> Code wise, I do a store.fetcher.load(webId) before I do a store.whether(statement) but it still shows that the dedicated ns.foaf('knows') statement exists, so the friend is not added because it shows it exists already. Not sure if store.fetcher.load should know of the changes on my pod already... or am I missing another function call in between?

@SharonStrats
Copy link
Contributor

SharonStrats commented Sep 3, 2021

Really great work! It's working very nicely. I will make further comments on your branch in your repository if I find code suggestions.

Ideas: I'm wondering in the case the user is not logged in, could the button say something like "login to add me to your friends".

it should be updated realtime. I will do some research on this, because I don't know off the top of my head how this happens. But it does happen with addresses for instance. If you click to add a new contact, it will pop back and tell you it was created. Actually I see what you mean, it changes somewhere else and do the changes take effect without a refresh... hmm I'm not sure about this.

Copy link
Contributor

@SharonStrats SharonStrats left a comment

Choose a reason for hiding this comment

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

Really great work! So exciting!

some additional more general suggestions. Right now there don't seem to be unit tests in general, but for this I think it would be good to have them. Just for your helper functions. So if you do create the folder friends and you have the helper.ts, you could just have one unit test file helper.test.ts to test your helper functions.

Awesome work 🥳!

//TODO create dedicated component in UI
const positiveFrontendMessageDiv = <HTMLDivElement>document.createElement("div");
positiveFrontendMessageDiv.setAttribute(
"style",
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case maybe it's appropriate to add a message style to baseStyles and add it in the same way as styles = .. styleMap. If there are specific styles to friends you can also create another file called perhaps friendStyles.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here, see @timbl comment in Gitter regarding the styles should be from solid-ui and use setStyle(). I will see if I can find an example and add here. The file to find them in is solid-ui/src/style.js and you can add a style here that you need. I see something called messageBodyStyle... this is something to look at although I don't know if it's exactly what you need. I will do some more research for this as well, as we definitely have errors in other places. I'm going to make another ticket though for styles in general as to make it easier to read in solid-ui I think we should perhaps have more than one style.js file going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually couldn't find many examples but here is one head = table.appendChild(dom.createElement('tr')); style.setStyle(head, 'autocompleteRowStyle'); // textInputStyle or. I also think solid-ui/src/messageArea.js is worth checking out for error messages. I can't be sure about this but something to look into.

Copy link
Member Author

@timea-solid timea-solid Sep 6, 2021

Choose a reason for hiding this comment

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

regarding styles, i did not change so much because it will cause inconsistencies on the Profile Pane. I propose opening an additional ticket with topic - align profile pane styles with UI - which goes beyond this button.

src/addMeToYourFriends.ts Outdated Show resolved Hide resolved
src/addMeToYourFriends.ts Outdated Show resolved Hide resolved
src/addMeToYourFriends.ts Show resolved Hide resolved
src/addMeToYourFriends.ts Outdated Show resolved Hide resolved
src/addMeToYourFriends.ts Outdated Show resolved Hide resolved
src/addMeToYourFriends.ts Outdated Show resolved Hide resolved
@timea-solid timea-solid changed the title implemneted feature https://github.com/solid/profile-pane/issues/8 implemneted feature 'add me to your friends' https://github.com/solid/profile-pane/issues/8 Sep 6, 2021
@timea-solid timea-solid closed this Sep 6, 2021
@timea-solid timea-solid reopened this Sep 6, 2021
@timea-solid
Copy link
Member Author

Really great work! It's working very nicely. I will make further comments on your branch in your repository if I find code suggestions.

Ideas: I'm wondering in the case the user is not logged in, could the button say something like "login to add me to your friends".

it should be updated realtime. I will do some research on this, because I don't know off the top of my head how this happens. But it does happen with addresses for instance. If you click to add a new contact, it will pop back and tell you it was created. Actually I see what you mean, it changes somewhere else and do the changes take effect without a refresh... hmm I'm not sure about this.

I updated the button text as mentioned.
I also added more tests.
Regarding refresh - this is a TODO

@timea-solid
Copy link
Member Author

I changed the button message as follows:
Screenshot 2021-09-14 at 16 31 01
Screenshot 2021-09-14 at 16 31 16
Screenshot 2021-09-14 at 16 31 49

@timea-solid
Copy link
Member Author

refresh also implemented now

Copy link
Contributor

@timbl timbl left a comment

Choose a reason for hiding this comment

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

I’m inclined to merge it on the basis that as it is new functionality it won’t break any existing stuff even if it doesn’t work, and merging it the best way of getting people to play with it.

@timbl timbl merged commit 26e8e2b into SolidOS:main Oct 7, 2021
@timbl
Copy link
Contributor

timbl commented Oct 7, 2021

Merge despite checks failing because npm-publish failure at this stage is bogus, should not apply once merged.

@timea-solid timea-solid deleted the add-me-to-your-friends branch October 19, 2021 07:43
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.

4 participants