Skip to content
This repository was archived by the owner on Dec 19, 2023. It is now read-only.

feat: capture navigation with transition animation #51

Merged
merged 54 commits into from
Mar 14, 2022

Conversation

Jan-Stepien
Copy link
Contributor

Description

Capture navigation with transition animation

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • πŸ› οΈ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • βœ… Build configuration change
  • πŸ“ Documentation
  • πŸ—‘οΈ Chore

@Jan-Stepien Jan-Stepien linked an issue Feb 15, 2022 that may be closed by this pull request
setUpAll(() {
registerFallbackValue(FakeRoute());
});
group('RouteGenerator', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

In such case we would rather use mockingjay library that allows for mocking top-most Navigator. Would that work for you as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried using mockingjay, but this test is 'if navigator leads to correct page', so the only way to see that is to allow Navigator to do its job.
Mocking Navigator was not an option here.

I've tried to test it like a standalone class, but it the end its all context based, so needed to wrap it in widget.
As a result there are 2 ways out - i can see if the correct Page(widget) was rendered and ignore if Navigator was involved at all.
or
I can leave an observer as is and verify if it was called.

I like double checking so opt for leaving it as it is, but its ur call at the end

Base automatically changed from capture_thank_you_screen to main February 21, 2022 17:10
child: Column(
mainAxisAlignment: MainAxisAlignment.center,
children: [
const SizedBox(height: 32),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const SizedBox(height: 32),
const SizedBox(height: AppSpacing.md),

Let's update this in the future to use standardized spacings :)

Comment on lines +42 to +43
Text(
context.l10n.captureTitle,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Text(
context.l10n.captureTitle,
Text(
l10n.captureTitle,
final l10n = context.l10n;

Just a nit, but we can extract the l10n similarly to theme

@Jan-Stepien Jan-Stepien merged commit 00d960e into main Mar 14, 2022
@Jan-Stepien Jan-Stepien deleted the capture_navigation_flow branch March 14, 2022 11:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5.2 - Thank you screen implementation
3 participants