Skip to content

Add navigation abstraction layer to EditPostActivity #22067

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

Merged
merged 6 commits into from
Jul 30, 2025

Conversation

oguzkocer
Copy link
Contributor

Summary

Replaces direct ViewPager manipulation with a testable navigation abstraction layer.

Changes

  • EditPostDestination: Type-safe sealed class for navigation targets
  • EditPostNavigationViewModel: Centralized navigation state with LiveData
  • EditPostNavigator: Interface for future implementation flexibility
  • Integration: Updated EditPostActivity to use navigation abstraction
  • Tests: Comprehensive unit tests covering all navigation scenarios

Benefits

  • Navigation logic is now testable and centralized
  • Maintains full backward compatibility with existing ViewPager
  • Enables future migration to ViewPager2/Navigation Component
  • Clear separation between navigation logic and UI concerns

Testing Instructions

  1. Open EditPostActivity and navigate between Editor/Settings/PublishSettings/History tabs
  2. Verify back navigation works correctly (PublishSettings → Settings → Editor)
  3. Confirm menu items show/hide appropriately based on current tab
  4. Test that UI updates (titles, toolbar styling) work as before

- EditPostDestination: Type-safe sealed class for navigation destinations
- EditPostNavigator: Interface for navigation implementation abstraction
- Provides foundation for replacing ViewPager with flexible navigation
- Manages navigation state and events using LiveData
- Provides navigateTo() method accepting destination enum
- Handles back navigation logic with proper fallback hierarchy
- Includes debugging logs for navigation flow tracking
- Uses EditPostDestination.default() for type-safe fallbacks
- Tests all navigation methods and state transitions
- Covers complete user journey flows (Editor → Settings → PublishSettings)
- Verifies event emission for UI updates
- Tests edge cases like back navigation from Editor
- Ensures proper fallback behavior and type safety
- 11 test scenarios with 100% navigation logic coverage
- Set up navigation state observers to update ViewPager position and UI
- Replace direct ViewPager.currentItem assignments with editPostNavigationViewModel.navigateTo()
- Update back navigation logic to use navigation ViewModel with proper fallback
- Update menu visibility logic to use navigation state instead of ViewPager position
- Add helper methods updateViewPagerPosition() and updateUIForDestination()
- Maintain existing ViewPager functionality while adding navigation abstraction layer
- Remove onPageSelected callback that was duplicating UI updates
- Fix History destination to use correct liftOnScrollTargetViewId (empty_recycler_view)
- UI updates now handled exclusively by navigation system in updateUIForDestination()
- ViewPager is now purely for content display, not navigation state management
@oguzkocer oguzkocer added this to the 26.1 milestone Jul 29, 2025
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 29, 2025

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr22067-579b8fa
Commit579b8fa
Direct Downloadwordpress-prototype-build-pr22067-579b8fa.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 29, 2025

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr22067-579b8fa
Commit579b8fa
Direct Downloadjetpack-prototype-build-pr22067-579b8fa.apk
Note: Google Login is not supported on these builds.

Copy link

Copy link

codecov bot commented Jul 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.29%. Comparing base (ce4aa0a) to head (579b8fa).
⚠️ Report is 2 commits behind head on trunk.

Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #22067   +/-   ##
=======================================
  Coverage   39.29%   39.29%           
=======================================
  Files        2143     2143           
  Lines      101698   101698           
  Branches    15597    15597           
=======================================
  Hits        39958    39958           
  Misses      58187    58187           
  Partials     3553     3553           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@oguzkocer oguzkocer marked this pull request as ready for review July 29, 2025 23:58
@oguzkocer oguzkocer enabled auto-merge (squash) July 29, 2025 23:58
@oguzkocer oguzkocer requested review from adalpari and nbradbury July 30, 2025 00:18
@nbradbury nbradbury self-assigned this Jul 30, 2025
* Interface for handling navigation between different screens in the Edit Post flow.
* Provides abstraction over the underlying navigation implementation.
*/
interface EditPostNavigator {
Copy link
Contributor

Choose a reason for hiding this comment

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

AS says this interface is unused, which I confirmed by deleting it and successfully building the app. Is there a reason to keep 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.

This abstraction happened a little early in the process. I'll remove it in #22068 and re-add it when we need it. Sorry for the confusion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in eae9287 in #22068.

/**
* The main editor screen where users write and edit post content.
*/
object Editor : EditPostDestination()
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: AS recommends converting these objects into data objects.

data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting that my AS doesn't 😕

Addressed in 662b71a in #22068.

Copy link
Contributor

@nbradbury nbradbury left a comment

Choose a reason for hiding this comment

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

@oguzkocer Code looks and works fine. I left a couple of comments, but it's up to you whether they're worth addressing so I'll approve this. I do recommend asking Copilot for a review in future PRs - it usually does a great job finding issues. :shipit:

Update: Well, I hadn't noticed that auto-merge was enabled, so I guess if you want to address those comments you can do so in a subsequent PR.

@oguzkocer oguzkocer merged commit 43b0fb7 into trunk Jul 30, 2025
27 checks passed
@oguzkocer oguzkocer deleted the feature/edit-post-navigation-abstraction branch July 30, 2025 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants