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

Passkey list for react package #89

Merged
merged 11 commits into from
Dec 20, 2023
Merged

Passkey list for react package #89

merged 11 commits into from
Dec 20, 2023

Conversation

Aby-JS
Copy link
Contributor

@Aby-JS Aby-JS commented Dec 18, 2023

Notes to review this PR

  • Update folder structure for components. Now, folders are distributed between UI, Wrappers and Screen Components.
  • Created a HOC for CorbadoProvider which takes input of all the theming params and provide custom theming and translations.
  • Updated the translation file structure so that we can add translations for passkey list component.

@Aby-JS Aby-JS added the enhancement New feature or request label Dec 18, 2023
@Aby-JS Aby-JS self-assigned this Dec 18, 2023
@Aby-JS Aby-JS linked an issue Dec 18, 2023 that may be closed by this pull request
4 tasks
@incorbador
Copy link
Contributor

Uploading image.png…
I can not add a passkey once all passkeys have been deleted

@incorbador
Copy link
Contributor

incorbador commented Dec 18, 2023

image

The button should really be part of the component so he should have the same background as the rest of the component I think. And I would add a bit of padding to it (maybe padding: 0 3em?)

@Aby-JS
Copy link
Contributor Author

Aby-JS commented Dec 18, 2023

image The button should really be part of the component so he should have the same background as the rest of the component I think. And I would add a bit of padding to it (maybe padding: 0 3em?)

I wanted it to have BE similar to user's app.. but let me just make it part of the app and check it out.

@incorbador
Copy link
Contributor

When I try to create a passkey on a device where I already have one, I get an error in the networking console (this is also expected, our backend does not allow for having multiple passkeys on the same device). We should catch the error and show it to the user (maybe as a red string below the "Create Passkey" button.

@incorbador
Copy link
Contributor

Please add the "Passkeys" headline from the Figma

image

@Aby-JS
Copy link
Contributor Author

Aby-JS commented Dec 18, 2023

Please add the "Passkeys" headline from the Figma

image

it this actually part of UI? I thought this is more like a design header?

@incorbador
Copy link
Contributor

Please add the "Passkeys" headline from the Figma
image

it this actually part of UI? I thought this is more like a design header?

Yes, valid point. Let can you ask Vincent about that? (maybe also about the fact, if the

image The button should really be part of the component so he should have the same background as the rest of the component I think. And I would add a bit of padding to it (maybe padding: 0 3em?)

I wanted it to have BE similar to user's app.. but let me just make it part of the app and check it out.

Good point, I think you are right (I checked the Figma again) => let's leave it as it is 👍

@Aby-JS
Copy link
Contributor Author

Aby-JS commented Dec 18, 2023

@incorbador I have made all the requested changes and have added translations as well. I think we should handler error in the error handling PR since it would require changes in the errors.ts file and I want to avoid conflicts.

Copy link
Contributor

@incorbador incorbador left a comment

Choose a reason for hiding this comment

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

See comments above. Regarding the older comments:

  • for error handling I agree, let's have a small follow up ticket for that and work on it after we merged the error handling PR
  • for "Passkeys" headline: I asked Vincent, we don't have a strong opinion about it so let's not include it (so no TODO for you here)

packages/react-sdk/src/contexts/CorbadoContext.tsx Outdated Show resolved Hide resolved
packages/shared-ui/src/i18n/de.json Outdated Show resolved Hide resolved
packages/react/src/hooks/useCorbadoUIConfig.ts Outdated Show resolved Hide resolved
packages/react/src/hocs/CorbadoScreen.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@incorbador incorbador left a comment

Choose a reason for hiding this comment

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

Last remaining comment is only about missing translations. Did you already find a fix for the lerna run build:dev problem? After that we can merge this PR.

packages/shared-ui/src/i18n/en.json Show resolved Hide resolved
@Aby-JS Aby-JS merged commit 0157a62 into main Dec 20, 2023
1 check passed
@Aby-JS Aby-JS deleted the passkey_list_for_react_package branch December 20, 2023 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional components: PasskeyList
2 participants