-
Notifications
You must be signed in to change notification settings - Fork 6
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
Conversation
@theRealImy I shall try it tomorrow
|
I improved the code as suggested.
|
There seem to be 2 issues partly related :
|
I've improved the code based on the feedback.
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.
|
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. |
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.
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 🥳!
src/addMeToYourFriends.ts
Outdated
//TODO create dedicated component in UI | ||
const positiveFrontendMessageDiv = <HTMLDivElement>document.createElement("div"); | ||
positiveFrontendMessageDiv.setAttribute( | ||
"style", |
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.
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.
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.
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.
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.
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.
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.
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.
I updated the button text as mentioned. |
refresh also implemented now |
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.
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.
Merge despite checks failing because npm-publish failure at this stage is bogus, should not apply once merged. |
Added the new 'Add me to your friends' button under profile (see screenshot).
The test case with logged in user will change when auth changes.