Skip to content

Hmt 44/qr code component #40

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

Merged
merged 17 commits into from
Jun 18, 2024
Merged

Hmt 44/qr code component #40

merged 17 commits into from
Jun 18, 2024

Conversation

burtonjong
Copy link
Contributor

Not done yet, just need feedback on the look of it

Copy link
Contributor

@hannagracec hannagracec left a comment

Choose a reason for hiding this comment

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

Hey! I just took a quick look at the UI and the QR code looks super nice. In terms of the layout, maybe put all of the QR content including the text in a panel similar to the other profile pages on the Figma? Maybe something like this:

image

I think the vertical look is nice as well just a panel might help make it look cohesive with the other pages!

Copy link
Contributor

@nataliey16 nataliey16 left a comment

Choose a reason for hiding this comment

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

It looks great! I also agree with adding the panel like Hanna suggested!

@burtonjong burtonjong requested a review from AllanKoder June 12, 2024 18:28
Copy link
Contributor

@AllanKoder AllanKoder left a comment

Choose a reason for hiding this comment

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

Looks great and amazing. I should have made things clearer in the README and the sample. This MAC address stuff is complicated cryptography so it's pretty easy to get confused. Thanks for all the hard work!

Also, you can test to see if your QR code works by creating a FoodEvent in the admin page, and scanning the QR code (or inputting the value of the QR code) into the admin food ticket scanner page.

@burtonjong burtonjong requested review from ideen1 and AllanKoder June 16, 2024 05:30
@burtonjong
Copy link
Contributor Author

image
@ideen1
??? I have been using this context the entire time to get the user ID, but it does not work now? Is it because I have to merge main?

@ideen1
Copy link
Contributor

ideen1 commented Jun 16, 2024

@burtonjong it got changed to username from userSub. Merge main and try that. UserSub wasn't compatible with the Google OAuth flow. But username works universally, it's the exact same value

Copy link
Contributor

@AllanKoder AllanKoder left a comment

Choose a reason for hiding this comment

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

Make sure you test the script to make sure it is working with the food ticket scanner, but at the high level, it looks like it is good. Also, if it is a huge issue that the user code is too long, you could edit the crytography util class and cutoff the MAC generation code (as in the generated code string), then it should continue to work fine.

Copy link
Contributor

@ideen1 ideen1 left a comment

Choose a reason for hiding this comment

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

2 small changes. Also instead of adding ctclogo.jpg can you use the existing CTCicon svg?

@burtonjong burtonjong requested a review from ideen1 June 17, 2024 03:08
@ideen1
Copy link
Contributor

ideen1 commented Jun 17, 2024

2 small changes. Also instead of adding ctclogo.jpg can you use the existing CTCicon svg?

Also the logo change @burtonjong

Copy link
Contributor

@ideen1 ideen1 left a comment

Choose a reason for hiding this comment

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

LGTM

@burtonjong burtonjong merged commit b24067c into main Jun 18, 2024
4 checks passed
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.

5 participants