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

82/burger #83

Merged
merged 34 commits into from
Oct 9, 2024
Merged

82/burger #83

merged 34 commits into from
Oct 9, 2024

Conversation

acholyn
Copy link
Collaborator

@acholyn acholyn commented Sep 25, 2024

closes #82
also closes #90 , closes #91 , closes #97 ?

@acholyn acholyn requested a review from tcouch September 25, 2024 13:19
@tcouch
Copy link
Collaborator

tcouch commented Oct 1, 2024

Does this work for you at the moment? I encountered an error related to setting alt text on button icons in the login form, then when I commented out those lines it complained about an invalid React hook call?

Edit: should have said this is when I click the Login button in the burger menu

@acholyn
Copy link
Collaborator Author

acholyn commented Oct 1, 2024

I am also getting the error when clicking the login - it should be showing a login dialog, shouldn't it? I'll have a go tomorrow

@acholyn acholyn marked this pull request as draft October 2, 2024 09:10
@acholyn acholyn marked this pull request as ready for review October 3, 2024 09:39
@tcouch
Copy link
Collaborator

tcouch commented Oct 3, 2024

Slight problem where the login dialog doesn't fit on my laptop screen. It's ok if I make the window narrower.

image

@tcouch
Copy link
Collaborator

tcouch commented Oct 3, 2024

A work around to not receiving the text messages is to get the lamda to print the code and get it from cloudwatch. I found the following errors doing this though:

  1. SmsInput -> handleSubmit function doesn't retrieve the code from the form before adding it to data. You need something like:
let formData = new FormData(e.target);
let code = formData.get("input");
  1. With that in place, clicking the thumbs up icon to submit the code doesn't work, pressing enter works though
  2. When the code gets sent to the API I get the following error: Error responding to SMS challenge InvalidParameterException: Missing required parameter USERNAME. When I logged the data being passed to respondToSMSChallenge to the console, phoneNumber was undefined. I think this is because line 89 you're calling getSMSVerificationCode with two parameters instead of three. I don't see setIsDialogVisible in LoginForm though...

</details>
<div className="bm-item bm-links">
<a
href="https://github.com/UCL/kapta-mobile"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Best practice would probably be to make this a build variable, but not a biggy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I close the browser then reload the page it's not showing me as logged in in the burger menu. The tokens are stored in localStorage though, so that mechanism to check on page load needs to be in place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah ok

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

have added it to the main menu component since that's where the button will display

const whatsappMapsUrl =
"https://uclexcites.blog/2024/06/26/whatsapp-maps-connecting-users-and-producers-of-ground-information/";
const extremeCitizenUrl = "https://www.youtube.com/watch?v=IgQc7GQ1m_Y";
const marcosUrl =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like there should be a better, structured way to organise contributor details so we can iterate over a list of objects and display name, role, link etc. Maybe one for another issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

have we opened another issue or not yet?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done #100

@acholyn acholyn merged commit 65d5585 into staging Oct 9, 2024
@acholyn
Copy link
Collaborator Author

acholyn commented Oct 9, 2024

didn't quite work with amplify, so will redo the merge

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.

2 participants