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

Reset password #16

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

Reset password #16

wants to merge 48 commits into from

Conversation

alicendeh
Copy link
Contributor

@alicendeh alicendeh commented Aug 3, 2022

OUTPUT
Screen Shot 2022-08-03 at 07 34 55

Screen Shot 2022-08-03 at 07 35 12

Screen Shot 2022-08-03 at 07 35 27

@tuxology tuxology requested a review from kamthamc August 22, 2022 21:18
Copy link

@kamthamc kamthamc left a comment

Choose a reason for hiding this comment

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

Translations are missing, please use theme colors. And some minor suggestions. Apart from that it looks good to me

*
* @todo - describe method's signature
*/
export const toggleSave = ({ id, token }) => {

Choose a reason for hiding this comment

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

May be tokens can be retrieved in request method instead of passing it all over


const styles = StyleSheet.create({
main: {
backgroundColor: 'rgba(250, 186, 186, 0.2)',

Choose a reason for hiding this comment

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

I think some files are are overlapping in #15 PR, please check the comments in there

@@ -32,14 +24,14 @@ const Profile = () => {
style={[
styles.card,
{
backgroundColor: activeTab === item.title ? '#000' : '#fff',
backgroundColor: activeTab === item.title ? "#000" : "#fff",

Choose a reason for hiding this comment

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

please use theme colors

<WebView
originWhitelist={['*']}
source={{
html: `

Choose a reason for hiding this comment

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

please avoid ifram if you can directly set the url

Comment on lines 114 to 123
Object.entries(userData).map((element) => {
if (
element[1] === '' &&
element[0] !== 'email' &&
element[0] !== 'phone' &&
element[0] !== 'bio'
) {
err.push(`We need your ${element[0]} to proceed`);
}
});

Choose a reason for hiding this comment

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

Suggested change
Object.entries(userData).map((element) => {
if (
element[1] === '' &&
element[0] !== 'email' &&
element[0] !== 'phone' &&
element[0] !== 'bio'
) {
err.push(`We need your ${element[0]} to proceed`);
}
});
Object.entries(userData).map(([key, value]) => {
if (
!value.trim().length &&
key !== 'email' &&
key !== 'phone' &&
key !== 'bio'
) {
err.push(`We need your ${element[0]} to proceed`);
}
});

shadowRadius: 2.22,
elevation: 3,
borderWidth: 2,
borderColor: '#eee',

Choose a reason for hiding this comment

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

please use theme colors

...user?.myFollowingList?.results,
]);
}
}, [user?.myFollowingList]);

Choose a reason for hiding this comment

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

please use fetchAllFollowers then block to set state instead of useEffect. Please avoid using dispatch in the components

Copy link
Member

@srish srish left a comment

Choose a reason for hiding this comment

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

I tested this PR. Leaving a few issues that I encountered:

  • A text says "check" below the phone field. It looks like an error; perhaps we should remove it. See screenshot.
  • What does it mean for a user to provide a strong password? Let's say something in a helper text about what we expect from users in the password field.
  • In the country data, I selected "United States" and got the following error message "Object=United States" doesn't exist. The moment I corrected it, the error disappeared, and the account got created. I think a more friendlier message might help and also we need to look into why this error message was shown in the first place.
  • Lastly, when I try to logout I get an error. Is logout suppose to work on this PR?

IMG_5012

IMG_5013

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.

3 participants