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

Improve README (#722) #765

Closed
wants to merge 3 commits into from

Conversation

barrybecker4
Copy link

@barrybecker4 barrybecker4 commented Sep 2, 2024

Some fixes to the README based on https://docs.google.com/document/d/1Ep7Xucqt-Y6OXiCO_LNOTwUep_DMgVfxy6V1Fg5AwzM/edit.

Also updated libx11 in the ui docker file to avoid an error.

@barrybecker4 barrybecker4 requested review from a team as code owners September 2, 2024 17:20
@barrybecker4 barrybecker4 changed the title Fix readme (#722) Improve README (#722) Sep 8, 2024
@ottomorac ottomorac requested review from x1m3 and amonsosanz September 9, 2024 12:51
@ottomorac
Copy link
Member

Thanks @barrybecker4 for the contribution.

@x1m3 Please review the contribution. Barry has been interacting with me and Ram in one our client conversations. He has confirmed that the PR is now ready for review.

@guillermovahi
Copy link

@barrybecker4 can you please provide the tutorial mentioned on this doc (https://docs.google.com/document/d/1Ep7Xucqt-Y6OXiCO_LNOTwUep_DMgVfxy6V1Fg5AwzM/edit) where it should be explained how to integrate PrivadoID SDK into a react app where a graduate student can apply for a job on the platform?
Thanks in advance

@ottomorac
Copy link
Member

ottomorac commented Sep 12, 2024

@barrybecker4 can you please provide the tutorial mentioned on this doc (https://docs.google.com/document/d/1Ep7Xucqt-Y6OXiCO_LNOTwUep_DMgVfxy6V1Fg5AwzM/edit) where it should be explained how to integrate PrivadoID SDK into a react app where a graduate student can apply for a job on the platform? Thanks in advance

Hi @guillermovahi . Not sure what you are asking. The google doc for the tutorial is already public. I gave you a follow on X if you want to chat via DM in more detail.

@barrybecker4
Copy link
Author

barrybecker4 commented Sep 14, 2024

@guillermovahi , I added that link in the README, but that doc contains nearly duplicate setup instructions. It would be nice to consolidate all the slightly different setup instructions into one source of truth. Also it refers to integration into a react app as a follow on tutorial, but never provides a link to it.
This is the commit that adds the link and does some other clean up.

@x1m3
Copy link
Contributor

x1m3 commented Sep 20, 2024

We are about to do a new release and there will be significant changes in the README file. We will consider your requests and try to include it.

@barrybecker4 @guillermovahi

@x1m3
Copy link
Contributor

x1m3 commented Oct 31, 2024

Hi @barrybecker4 @guillermovahi

New v3.0.0 release includes many changes and i guess your suggestions are covered there.

Please, for contributions follow guidelines in contrib.md. We usually cannot accept direct merges to main branch. The desired branch is develop.

Any suggestion and contrib is always welcome

@x1m3 x1m3 closed this Oct 31, 2024
@barrybecker4
Copy link
Author

@x1m3 @guillermovahi I'm not sure if any of my changes were actually incorporated. For example

  • There are no additional instructions on how to use or configure ngrok or localtunnel.
  • The license link at the bottom is still broken.
  • Adding additional University credential demo link

I will give the new instructions a try and see if they are any easier, but it would be nice if some of my suggestions could be incorporated.

@amonsosanz
Copy link
Contributor

Hi @barrybecker4.

There are no additional instructions on how to use or configure ngrok or localtunnel.

We do mention "use a tool like Localtunnel for testing purposes" in the readme. I don't think our readme should include a deeper level of detail. You can go to the documentation of these tools to get the details on how to use them. Also, these third-party tools, if ever used, should be used only in the testing phase, never in production. I fear that giving them more relevance in the readme could mislead some inexperienced users. But it's my opinion, you might be right. Also, mention that we have this guide in our FAQ. Maybe we can link it emphasizing that it should be only used for testing? By the way, I have just opened a PR to update that guide that became obsolete after this release.

The license link at the bottom is still broken.

This was fixed here. You can see the fix in the develop branch.

Adding additional University credential demo link

I guess we can add that link.

@barrybecker4
Copy link
Author

Thanks. I will give the new instructions another try this weekend.

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