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

Enhance documentation and set better diagram with some pictures #9

Merged
merged 10 commits into from
Aug 16, 2019

Conversation

AhmedBHameed
Copy link
Collaborator

Hello @firsttris ,

Hope you are doing well.

May you please review and confirm those changes when you have time ?

All best wishes.

@firsttris
Copy link
Owner

firsttris commented Aug 15, 2019

Hey @AhmedBHameed

good work, i really appreciate the effort you put in.

here are some advices to your pull-request. you don't have todo this just if you like it ;)

1.) Examples
"For instance, those code can be used to read from your NFC card and log some information on the screen"

would it not be better to put this example in the test folder and link it in the readme, so that people can directly run it?
also we would have only one source for those "test's / examples".

what about those others tests are they still valid and useful?
then there is this issue #5 with an example of NTAG213, should we maybe update the dumpNTAG213.js file?

2.) Demonstration
i like it, very good.
what do you think about the idea to put pictures of the demonstration in a own Markdown File and link it in the Readme File? Because the Readme tend to get big.

3.) Video
this is super nice.
it would be even better if we could directly embed the Video, there are two possible ways i could think of

option 1 would be to convert it to a gif and embed it.

option 2 would be upload it somewhere like youtube and make a link [pictureOfVideo](urlToYoutube).

i think most people are to lazy and will not download and play it (to much steps)

best wishes
Tristan

@AhmedBHameed
Copy link
Collaborator Author

AhmedBHameed commented Aug 15, 2019

Hello @firsttris ,

  1. I agree with you, i removed the example and kept oly links to test. Regarding other test, unfortunately, my chip is only for reading and nu writable chip so not sure how to test those without having writable chip in hand. so i can't verify them :(

Regarding to do, i have one nice idea but i'm afraid from writing it inside. I don't know where to find time and do it again, I did it before with bython but i want to make it with node again. Will see later what can i do.

  1. Yes good idea i spitted them in a separate folder.

  2. You can check the link for the video now.

Sorry for those mistakes, my experience with publishing things in development hehe.

Regards.
Ahmed

@AhmedBHameed
Copy link
Collaborator Author

Also what do you thing about updating npm docs ? That would also participate in distributions of the module.

@firsttris
Copy link
Owner

npm docs will get updated with git readme once we released a new version

@firsttris
Copy link
Owner

do you think this extended wiring picture will be confusing for newbie users?

https://github.com/firsttris/mfrc522-rpi/raw/docs/wiki/mfrc522-node.png

in comperence to the old one

https://github.com/firsttris/mfrc522-rpi/raw/master/wiki/rpi-mfrc522-wiring2.PNG

i mean if you are new you would probably just connect it to the PI, without buzzer

my understanding is that both options are possible.

would it not be better to put both wiring pictures in the readme?

Saying this one is the simple wiring, and the other is the extended version with buzzer.

@AhmedBHameed
Copy link
Collaborator Author

I was thinking the new schema might be easier!!
Hmmm, better to re-think again. Ok will retrieve back the base connection and add some notes for extra buzzer feature.

Thank you for noting.

@AhmedBHameed
Copy link
Collaborator Author

I added new schema as optional way for interfacing.

@firsttris
Copy link
Owner

nice sir, i did some polishing as well, are you okay with it?

i would be rdy to merge this

@AhmedBHameed
Copy link
Collaborator Author

Sound good for me. Thank you for your nice polish.

@firsttris firsttris merged commit 288cc7a into master Aug 16, 2019
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.

None yet

2 participants