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

feat(react-hooks): extend credential providers with format data #146

Closed

Conversation

amanji
Copy link
Contributor

@amanji amanji commented Aug 11, 2022

Signed-off-by: Akiff Manji [email protected]

@codecov-commenter
Copy link

Codecov Report

Merging #146 (f08e9f7) into main (d60a5f9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #146   +/-   ##
=======================================
  Coverage   87.07%   87.07%           
=======================================
  Files          45       45           
  Lines         681      681           
  Branches       84       84           
=======================================
  Hits          593      593           
  Misses         88       88           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Nice!

I've updated the title to be feat instead of refactor as there is a change in functionality, and otherwise a new release won't get triggered for just a refactor


export const useCredentials = () => {
export const useCombinedCredentials = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe useCredentialsWithFormatData()? Or do you think that is too long?

combined doesn't really give any context on what is combined

@TimoGlastra TimoGlastra changed the title refactor(react-hooks): extend credential providers with format data feat(react-hooks): extend credential providers with format data Aug 11, 2022

interface CombinedRecord<R extends BaseRecord> {
record: R
formatData: GetFormatDataReturn<[IndyCredentialFormat]>
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to only specifically do IndyCredentialFormat I think we should name it accordingly.

I also implemented this locally for some projects and just made the formatData a separate hook.

const formattedCredential = useCredentialFormatDataById(credentialId)

or

const formattedCredential = useCredentialFormatData()

Which could then optionally take a generic for the format data, i.e. IndyCredentialFormat. I could also create a PR with this code (and for proofs.)

@amanji
Copy link
Contributor Author

amanji commented May 16, 2023

I think this PR is probably quite out of sync with what's in AFJ today.

IMHO if you have working code that expands on this and it's not adding unnecessary work for you I'd say maybe we go with that instead.

Let me know. I'd be happy to review once it's ready.

@berendsliedrecht
Copy link
Contributor

Opened #231 that supersedes this.

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.

4 participants