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.

Sign up for GitHub

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

Jump to bottom

Pp 628 implement default and custom bottom bar navigation #574

Merged
merged 34 commits into from
Jul 22, 2024
Merged

Pp 628 implement default and custom bottom bar navigation #574

merged 34 commits into from
Jul 22, 2024

Conversation

mrkulik
Copy link
Contributor

@mrkulik mrkulik commented Jul 10, 2024

Simulator Screenshot - iPad Pro (11-inch) (4th generation) - 2024-07-10 at 10 46 45
Simulator Screenshot - iPhone SE (1st generation) - 2024-07-10 at 10 47 53
Simulator Screenshot - iPhone SE (3rd generation) - 2024-07-10 at 10 46 43

@mrkulik mrkulik self-assigned this Jul 10, 2024
ValentinaIancu-Gini
ValentinaIancu-Gini reviewed Jul 10, 2024
View reviewed changes
Copy link
Contributor

@ValentinaIancu-Gini ValentinaIancu-Gini left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@mrkulik Great job!

Please see my suggestions.

As well I found some small UI issues like:

  1. There is an extra back button on the screen if the bottom bar navigation is activated.

  2. The space between the "Proceed" button and the bottom of the screen is too small on our screen and in the design it seems to have an extra space. Please check this also.

IMG_0193

Can you please also update the Settings screen with a toggle for custom bottom bar navigation for this screen? Vlad will need it to test it. I tested this very fast and I can provide some code but it seems not to be complete.

Bildschirmfoto 2024-07-10 um 18 31 22

...kSDK/Sources/GiniBankSDK/Core/Skonto/BottomNavigation/DefaultSkontoBottomNavigationBar.swift Outdated Show resolved Hide resolved
...kSDK/Sources/GiniBankSDK/Core/Skonto/BottomNavigation/DefaultSkontoBottomNavigationBar.swift Outdated Show resolved Hide resolved
CaptureSDK/GiniCaptureSDK/Sources/GiniCaptureSDK/Core/Custom views/GiniBarButton.swift Show resolved Hide resolved
mrkulik added 19 commits July 11, 2024 12:00
@mrkulik
feat(GiniBankSDK): remove extra back button when bottom navigation is on
918ebb6 
PP-628
@mrkulik
feat(GiniBankSDK): update localization strings
772242f 
PP-628
@mrkulik
feat(GiniBankSDK): change bottom padding for custom navigation bar
a07db01 
PP-628
@mrkulik
feat(GiniBankSDKExample): CustomSkontoBottomNavigationBar & Adapter
a2f1b52 
PP-628
@mrkulik
Merge branch 'PP-51-Delivery-Skonto-iOS' into PP-628-Implement-defaul…
12eba70 
…t-and-custom-bottom-bar-navigation
@mrkulik
feat(GiniBankSDK): Temporary remove help action for bottom navigation
b43985d 
PP-51
@mrkulik
feat(GiniBankSDKExample): Temporary remove help action for custom bot…
fa3302d 
…tom navigation

PP-628
@mrkulik
feat(GiniBankSDKExample): remove updateProceedButtonState from bottom…
320436d 
… navigation adapter

PP-628
@mrkulik
feat(GiniBankSDKExample): add tests for Skonto bottom bar switch option
3f4ee9c 
PP-628
@mrkulik
feat(GiniBankSDKExample): remove unused methods for proceed button fr…
6ebd20f 
…om custom bottom bar

PP-628
@mrkulik
feat(GiniBankSDKExample): set numberOfLines for skonto label in custo…
7dbdc5b 
…m bottom bar

PP-628
@mrkulik
feat(GiniBankSDK): set numberOfLines for skonto label in default bott…
efd172b 
…om bar

PP-628
@mrkulik
feat(GiniBankSDK): set correct price formatting for default bottom bar
5d11d90 
PP-628
@mrkulik
feat(GiniBankSDKExample): replace priceWithCurrencySymbol with priceW…
e7fff46 
…ithCurrencyCode

PP-628
@mrkulik
feat(GiniBankSDK): add divider view to default bottom bar. Move corne…
56cda2a 
…rRadius to Constants

PP-628
@mrkulik
Merge branch 'PP-51-Delivery-Skonto-iOS' into PP-628-Implement-defaul…
3271224 
…t-and-custom-bottom-bar-navigation
@mrkulik
feat(GiniBankSDK): remove unneed method updateProceedButtonState & se…
b167b6b 
…tProceedButtonState

PP-628
@mrkulik
feat(GiniBankSDK): add accessibility values for default bottom bar
dc5a3a9 
PP-628
@mrkulik
feat(GiniBankSDK): update layout & constraints for default bottom nav…
680b4ff 
…igation bar

PP-628
ValentinaIancu-Gini
ValentinaIancu-Gini reviewed Jul 16, 2024
View reviewed changes
...kSDK/Sources/GiniBankSDK/Core/Skonto/BottomNavigation/DefaultSkontoBottomNavigationBar.swift Outdated Show resolved Hide resolved
BankSDK/GiniBankSDK/Sources/GiniBankSDK/Core/Skonto/SkontoViewController.swift Outdated Show resolved Hide resolved
...ple/GiniBankSDKExample/SettingsViewController/SettingsViewController+SwitchOptionModel.swift Show resolved Hide resolved
...ple/GiniBankSDKExample/SettingsViewController/SettingsViewController+SwitchOptionModel.swift Show resolved Hide resolved
...ple/GiniBankSDKExample/SettingsViewController/SettingsViewController+SwitchOptionModel.swift Show resolved Hide resolved
...DK/GiniBankSDKExample/GiniBankSDKExample/SettingsViewController/SettingsViewController.swift Show resolved Hide resolved
...DK/GiniBankSDKExample/GiniBankSDKExample/SettingsViewController/SettingsViewController.swift Show resolved Hide resolved
BankSDK/GiniBankSDKExample/Tests/SettingsViewControllerTests.swift Show resolved Hide resolved
@ValentinaIancu-Gini
Copy link
Contributor

ValentinaIancu-Gini commented Jul 16, 2024 •
edited
Loading

@mrkulik Looks good.
I left some minor suggestions.

Also now that we hide the Help button the bottom navigation bar is looking awkward. Maybe is good to insert a spacer to the right of the Proceed button until the next phase when we add the Help button. What do you say?
Bildschirmfoto 2024-07-16 um 09 15 47

@mrkulik mrkulik force-pushed the PP-628-Implement-default-and-custom-bottom-bar-navigation branch from 5d3df42 to f86fbf9 Compare July 16, 2024 08:55
Base automatically changed from PP-51-Delivery-Skonto-iOS to PP-602-iOS July 16, 2024 08:56
ValentinaIancu-Gini
ValentinaIancu-Gini approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@ValentinaIancu-Gini ValentinaIancu-Gini left a comment

Choose a reason for hiding this comment

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

@mrkulik Looks good. Thank you!

I will wait for all the checks to pass and I can give a build for Vlad to test this pr changes.

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
24.1% Duplication on New Code

See analysis details on SonarCloud

@mrkulik mrkulik merged commit ff5d5e0 into PP-602-iOS Jul 22, 2024
17 checks passed
@mrkulik mrkulik deleted the PP-628-Implement-default-and-custom-bottom-bar-navigation branch July 22, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValentinaIancu-Gini ValentinaIancu-Gini ValentinaIancu-Gini approved these changes

Assignees

@mrkulik mrkulik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mrkulik @ValentinaIancu-Gini