feat(deriv_auth): [UPM-547] added deriv_passkeys to deriv_auth #488
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
feat that adds a single entry to the auth package
…ges into deriv-auth/feat/single_entry
change deriv_api to dev branch and package_info_plus to version 4
…ges into deriv-auth/feat/single_entry # Conflicts: # packages/deriv_auth/lib/deriv_auth.dart # packages/deriv_auth/lib/features/login/presentation/layouts/deriv_login_layout.dart # packages/deriv_auth/lib/features/signup/presentation/layouts/deriv_signup_layout.dart # packages/deriv_auth/pubspec.yaml
bassam-deriv
changed the title
There was a problem hiding this comment.
why is this added inside social_auth folder?
There was a problem hiding this comment.
because it's being used in the social auth service:
BlocProvider(
create: (context) => SocialAuthCubit(
socialAuthService: DerivSocialAuthService(
client: HttpClient(),
connectionInfo: DerivAuthConnectionInfo(),
),
),
),
There was a problem hiding this comment.
It is passed in DerivAuthCubit as well. DerivAuthConnectionInfo is for auth endpoint - oauth.deriv.com, it should be in somewhere common folder.
Comment on lines
3
to
7
| import 'package:deriv_passkeys/data/data_sources/deriv_passkeys_data_source.dart'; | ||
| import 'package:deriv_passkeys/data/mappers/deriv_passkeys_mapper.dart'; | ||
| import 'package:deriv_passkeys/data/repositories/deriv_passkeys_repository.dart'; | ||
| import 'package:deriv_passkeys/interactor/services/deriv_passkeys_service.dart'; | ||
| import 'package:deriv_passkeys/presentation/states/bloc/deriv_passkeys_bloc.dart'; |
There was a problem hiding this comment.
Not related to this PR but can you create a barrel file for deriv_passkeys package?
There was a problem hiding this comment.
Yes i realized i should when i was implementing, thank you for the suggestion!
Comment on lines
25
to
40
| deriv_passkeys: | ||
| git: | ||
| url: [email protected]:regentmarkets/flutter-deriv-packages.git | ||
| path: packages/deriv_passkeys | ||
| ref: c9b906bc20edbc94d0ca99f2211b142efa3aaf2e | ||
|
|
||
| dependency_overrides: | ||
| flutter_deriv_api: | ||
| git: | ||
| url: [email protected]:deriv-com/flutter-deriv-api.git | ||
| ref: 64ad3673598ad807b43f751fc2ec7bc5ef01203e | ||
| deriv_ui: | ||
| git: | ||
| url: [email protected]:regentmarkets/flutter-deriv-packages.git | ||
| path: packages/deriv_ui | ||
| ref: deriv_ui-v0.0.6+2 |
There was a problem hiding this comment.
dont forget to change the dependencies before merge.
| /// Extension methods for capitalizing [String]. | ||
| extension Capitalize on String { | ||
| /// Capitalize the first letter of the string. | ||
| String get capitalize => '${this[0].toUpperCase()}${substring(1)}'; |
There was a problem hiding this comment.
This could have been added to StringExtension why did you create a new extension?
| child: Column( | ||
| mainAxisAlignment: MainAxisAlignment.center, | ||
| children: <Widget>[ | ||
| const ContinueWithPasskeyButton(), |
There was a problem hiding this comment.
This widget is solely for social auth as name suggested. Its better we have a new widget where you use this social auth panel and also add the passkey button and name the widget something else.
Comment on lines
82
to
96
| IconButton( | ||
| padding: EdgeInsets.zero, | ||
| iconSize: ThemeProvider.iconSize40, | ||
| icon: Opacity( | ||
| opacity: getOpacity(isEnabled: widget.isEnabled), | ||
| child: SvgPicture.asset( | ||
| _getSocialMediaIcon(socialAuthProvider), | ||
| package: 'deriv_auth', | ||
| InkWell( | ||
| child: Container( | ||
| padding: const EdgeInsets.symmetric( | ||
| horizontal: 16, | ||
| vertical: 12, | ||
| ), | ||
| decoration: BoxDecoration( | ||
| borderRadius: BorderRadius.circular(4), | ||
| border: Border.all( |
There was a problem hiding this comment.
Can you also add the opacity based on isEnabled.
|
Please fix the failing checks. |
* chore(release): publish packages - [email protected]+4 - [email protected] * [create-pull-request] automated change --------- Co-authored-by: GitHub Actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: sahani-deriv <[email protected]>
* feat(deriv_auth): single entry feat that adds a single entry to the auth package * chore(deriv_auth): improve some imports * chore(deriv_auth): change deriv-api to dev branch change deriv_api to dev branch and package_info_plus to version 4 * feat(deriv_auth): adding login page to single entry * feat(deriv_auth): single entry login and reset pass added * feat(deriv_auth): single entry signup page * chore: setting page fist attempt * feat(deriv_auith): single entry setting page * feat(deriv_auth): single entry reset password and merge conflicts * chore(deriv_auth): code review comments add docs and minor changes * chore(deriv_auth): change residence to userResidence * doc(deriv_auth): single entry doc and fix error handling * chore(deriv_auth): fixing the deriv reset pass layout test * chore(deriv_auth): change navigation to signupPage in verifyemailpage
ahrar-deriv
reviewed
Apr 1, 2024
| @@ -17,7 +17,9 @@ part 'deriv_auth_state.dart'; | |||
| /// and it is responsible for all login functionality. | |||
| class DerivAuthCubit extends Cubit<DerivAuthState> implements DerivAuthIO { | |||
| /// Initialize a [DerivAuthCubit]. | |||
| DerivAuthCubit({required this.authService}) : super(DerivAuthLoadingState()); | |||
| DerivAuthCubit({ | |||
| required this.authService, | |||
There was a problem hiding this comment.
do we need the trailing comma
There was a problem hiding this comment.
No, i had added another parameter but removed later that's why it's there.
ahrar-deriv
reviewed
Apr 1, 2024
| @@ -25,7 +25,7 @@ class LoginPageModel { | |||
| final Function(DerivAuthErrorState)? onLoginError; | |||
|
|
|||
| /// Callback to be called when user is logged in. | |||
| final Function(DerivAuthLoggedInState) onLoggedIn; | |||
| final Function(BuildContext, DerivAuthLoggedInState) onLoggedIn; | |||
There was a problem hiding this comment.
this makes this PR breaking change
There was a problem hiding this comment.
Since we're not going with the single entry implementation in deriv go i'll remove this change so that this PR doesn't have to be breaking.
bassam-deriv
changed the title
bassam-deriv
changed the title
packages/deriv_auth/pubspec.yaml
Outdated
Comment on lines
72
to
82
| dependency_overrides: | ||
| flutter_deriv_api: | ||
| git: | ||
| url: [email protected]:deriv-com/flutter-deriv-api.git | ||
| ref: dev | ||
| deriv_localizations: | ||
| git: | ||
| url: [email protected]:regentmarkets/flutter-deriv-packages.git | ||
| path: packages/deriv_localizations | ||
| ref: da897013ecd1f993ad4e696c1d0856d7ebf3758c | ||
|
|
packages/deriv_auth/pubspec.yaml
Outdated
| git: | ||
| url: [email protected]:regentmarkets/flutter-deriv-packages.git | ||
| path: packages/deriv_passkeys | ||
| ref: 4cb6ab174fc11aac2799475ce4cd874612f7e2da |
abedelaziz-deriv
approved these changes
May 20, 2024
…ve unused import in deriv_get_started_layout_test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clickup link:
Fixes issue: #
This PR contains the following changes:
Developers Note (Optional)
Pre-launch Checklist (For PR creator)
As a creator of this PR:
Reviewers
Pre-launch Checklist (For Reviewers)
As a reviewer I ensure that:
Pre-launch Checklist (For QA)
Pre-launch Checklist (For Maintainer)