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: add new oca branding 1.1 for credential list, proof request and verifier credential card #1405

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

Conversation

fc-santos
Copy link
Contributor

@fc-santos fc-santos commented Jan 22, 2025

Summary of Changes

This PR is part 1 out of 2 for the new OCA branding (1.1). It covers the credential card displayed in the ListCredentials Screen, in the Proof Request Screen and in the VerifierCredentialCard component.

This introduces a breaking change for anyone using the new Branding:

  • The credential card shadow comes now from the theme
  • The selected credential them also comes now from the theme

A PR is on progress at the RFC repository related to this new branding: hyperledger/aries-rfcs#864

Screenshots, videos, or gifs

ListCredentials Screen (with/without oca bundle):
image

ProofRequest Screen:
image

VerifierCrendentialCard (with/without oca bundle):
image
image

Breaking change guide

The following interface has two new properties. These are the values for the two new props:

const CredentialCardShadowTheme = {
  shadowColor: '#000',
  shadowOffset: {
    width: 1,
    height: 1,
  },
  shadowOpacity: 0.3,
} satisfies ViewStyle

const SelectedCredTheme = {
  borderWidth: 5,
  borderRadius: 15,
  borderColor: ColorPallet.semantic.focus,
} satisfies ViewStyle

export interface ITheme {
  /* .... */
  CredentialCardShadowTheme: ViewStyle
  SelectedCredTheme: ViewStyle
}

Related Issues

N/A

Pull Request Checklist

Tick all boxes below to demonstrate that you have completed the respective task. If the item does not apply to your this PR check it anyway to make it apparent that there's nothing to do.

  • All commits contain a DCO Signed-off-by line (we use the DCO GitHub app to enforce this)
  • If applicable, screenshots, gifs, or video are included for UI changes
  • If applicable, breaking changes are described above along with how to address them
  • Updated documentation as needed for changed code and new or modified features
  • Added sufficient tests so that overall code coverage is not reduced

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

Pro Tip 🤓

  • Read our contribution guide at least once; it will save you a few review cycles!
  • Your PR will likely not be reviewed until all the above boxes are checked and all automated checks have passed

@fc-santos fc-santos requested a review from a team as a code owner January 22, 2025 16:51
@fc-santos fc-santos changed the title Feat/add new oca branding 1 1 for cred list and proof req feat: add new oca branding 1 1 for cred list and proof req Jan 22, 2025
@fc-santos fc-santos changed the title feat: add new oca branding 1 1 for cred list and proof req feat: add new oca branding 1.1 for credential list, proof request and verifier credential card Jan 22, 2025
@fc-santos fc-santos marked this pull request as draft January 22, 2025 17:26
@fc-santos fc-santos marked this pull request as ready for review January 23, 2025 17:45
Signed-off-by: fc-santos <[email protected]>
Signed-off-by: fc-santos <[email protected]>
Copy link
Contributor

@al-rosenthal al-rosenthal left a comment

Choose a reason for hiding this comment

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

Is this going to conflict with this PR: 1320

credName={credName}
credDefId={credDefId}
schemaId={schemaId}
credential={credential as CredentialExchangeRecord}
handleAltCredChange={handleAltCredChange}
hasAltCredentials={hasAltCredentials}
proof
proof={isBranding10}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a credential with a branding type 1.0 always a proof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@al-rosenthal we want the new branding to have the same look as the one from the CredentialList screen. The prop was already true in the beginning since we are inside the if block:

if (proof) {
      return (
        <CredentialCard11
          displayItems={displayItems}
          style={isBranding10 ? { backgroundColor: ColorPallet.brand.secondaryBackground } : undefined}
          credName={credName}
          credDefId={credDefId}
          schemaId={schemaId}
          credential={credential as CredentialExchangeRecord}
          handleAltCredChange={handleAltCredChange}
          hasAltCredentials={hasAltCredentials}
          proof={isBranding10} // before it was just proof
          elevated
          credentialErrors={credentialErrors ?? []}
          brandingOverlayType={bundleResolver.getBrandingOverlayType()}
        />
      )
    }

@@ -112,6 +114,7 @@ const CredentialCard11: React.FC<CredentialCard11Props> = ({
])
const [helpAction, setHelpAction] = useState<GenericFn>()
const [overlay, setOverlay] = useState<CredentialOverlay<BrandingOverlay>>({})
const { styles, borderRadius, logoHeight } = useCredentialCardStyles(overlay, brandingOverlayType)
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably pass proof boolean into this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@al-rosenthal resolved!

@@ -1250,3 +1255,16 @@ export function generateRandomWalletName() {

return name
}

export function getSecondaryBackgroundColor(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this feels like it should be in theme file with other style related things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@al-rosenthal not really sure how to move this function to the theme.ts file since it takes some params (overlay and proof). I could rename the function though to be more clear its for use in the CredentialCard.

@fc-santos fc-santos mentioned this pull request Jan 24, 2025
4 tasks
@fc-santos
Copy link
Contributor Author

Is this going to conflict with this PR: 1320

@al-rosenthal I've just requested that PR to be closed, this PR should be easier to review compared to the other one.

@fc-santos fc-santos requested a review from MosCD3 January 24, 2025 19:58
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.

2 participants