Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Add nfc ui #488

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

Add nfc ui #488

wants to merge 2 commits into from

Conversation

kobieaw
Copy link

@kobieaw kobieaw commented May 4, 2023

I added the .svg file and the button and image that has the "Read Card" text. Still need to add the business logic functionality. I also had pulled all of the other changes from the main branch. That will show up.

@Extheoisah
Copy link
Contributor

Hi @kobieaw
Can you open the PR against the main branch and not the previous nfc PR?
It makes it difficult to review the code because the diff isn't compared against the main branch, hence, I can't know what changes are new or already existed.

@kobieaw kobieaw changed the base branch from feat-add-nfc to main May 4, 2023 14:08
@kobieaw
Copy link
Author

kobieaw commented May 4, 2023

Hey @Extheoisah,
I changed the base to main. This should work, if there is a complication let me know and I can make a new PR. Thanks

@Extheoisah
Copy link
Contributor

Hey @Extheoisah, I changed the base to main. This should work, if there is a complication let me know and I can make a new PR. Thanks

Yeah. This works. However, i only see the ui changes you've made. Is the logic for the nfc still in progress?

@kobieaw
Copy link
Author

kobieaw commented May 4, 2023

Yes, the bounty description just said to PR with the UI first. And in the description it said to do a different PR with the code implementation. Correct me if I'm wrong

@nicolasburtey
Copy link
Member

Yes, the bounty description just said to PR with the UI first. And in the description it said to do a different PR with the code implementation. Correct me if I'm wrong

there is not much currently in this PR. the idea when we talk about the UI, is not to add just the button, but think about the overall experience, how things work when the payment is completed/pending/failed with NFC. do you haptic feedback, sound or something along those lines.

but maybe that's easier to magic the logic altogether in this case @kobieaw

@nicolasburtey
Copy link
Member

Yes, the bounty description just said to PR with the UI first. And in the description it said to do a different PR with the code implementation. Correct me if I'm wrong

there is not much currently in this PR. the idea when we talk about the UI, is not to add just the button, but think about the overall experience, how things work when the payment is completed/pending/failed with NFC. do you haptic feedback, sound or something along those lines.

but maybe that's easier to magic the logic altogether in this case @kobieaw

btw, the svg has some text in it. I guess this is an oversight.

@kobieaw
Copy link
Author

kobieaw commented May 12, 2023

Yes, the bounty description just said to PR with the UI first. And in the description it said to do a different PR with the code implementation. Correct me if I'm wrong

there is not much currently in this PR. the idea when we talk about the UI, is not to add just the button, but think about the overall experience, how things work when the payment is completed/pending/failed with NFC. do you haptic feedback, sound or something along those lines.

but maybe that's easier to magic the logic altogether in this case @kobieaw

Sounds good, yea I didn't know if you guys wanted all the other stuff like payment status and haptic feedback in the UI/UX PR or in logic. I'll update this PR. I'll also figure out how to get rid of the text at the bottom of the svg.

@kobieaw
Copy link
Author

kobieaw commented Jun 1, 2023

Got the .svg out in a commit last week fyi. Fixed the issue with me not being able to test and now adding other stuff like payment status

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants