Skip to content

Conversation

@jaclync
Copy link
Contributor

@jaclync jaclync commented Oct 1, 2025

Part of WOOMOB-935

Just one review is required.

Description

This PR fixes most of Periphery warnings from POS modularization #16176 by removing unused code and refactoring code that was flagged but is actually in use (like from using a private API).

Key changes:

  • Removed unused DynamicHStack and DynamicVStack components
  • Removed unused backgroundColor parameter from RasterizedShadowBackground
  • Refactored POSSearchTextFieldStyle from using private API _body to a proper ViewModifier
  • Fixed warnings in ConnectivityObserver protocol by removing unused functions
  • Fixed warnings in barcode scanning functionality
  • Removed various unused functions and properties across the POS code

These changes help improve code maintainability by removing dead code and ensuring we're not using private APIs.

Steps to reproduce

Regression testing:

  1. Enter POS
  2. Verify POS search functionality works as before (search bar UI/UX, search history)
  3. Verify shadow styles render correctly in POS UI
  4. Test barcode scanning in POS
  5. Verify connectivity observer works as before (offline banner)

Testing information

  • I tested the above in iPad Pro 11in 3rd generation, iOS 26.0, from the installable build

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@dangermattic
Copy link
Collaborator

dangermattic commented Oct 1, 2025

2 Warnings
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@jaclync jaclync added type: technical debt Represents or solves tech debt of the project. feature: POS labels Oct 1, 2025
@jaclync jaclync added this to the 23.4 milestone Oct 1, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 1, 2025

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Numberpr16185-33f6e09
Version23.3
Bundle IDcom.automattic.alpha.woocommerce
Commit33f6e09
Installation URL7a5djsfoakg6o
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jaclync jaclync changed the title Fix Periphery warnings [Woo POS] Modularization: fix most Periphery warnings Oct 1, 2025
Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

Thank you very much for this! I tested the described cases, it continues to work.

Respect for addressing all these past warnings. I remember I just rebuilt Periphery cache to avoid this but of course this is much better and beneficial when moving to POS module. We will be in much cleaner state.

import enum Yosemite.PointOfSaleBarcodeScanError

protocol PointOfSaleAggregateModelProtocol {
var orderStage: PointOfSaleOrderStage { get }
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I guess it's a fair call from Periphery. We include many of these in the protocol, but never really use them when injecting aggregate modal as a protocol. We can always included when needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I think it's good to only include what's used. It could also help break up the big protocol into smaller ones for different use cases from the aggregate model.

/// Text field style for search fields that includes a magnifier icon and clear button
struct POSSearchTextFieldStyle: TextFieldStyle {
/// View modifier for search fields that includes a magnifier icon and clear button
struct POSSearchTextFieldModifier: ViewModifier {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the commit is:

Refactor POSSearchTextFieldStyle to a view modifier to avoid using private API _body

That's interesting! Did we use an disallowed way to create our own text field style? I looked up online, and this is how it's created in the examples. Should we just ignore this file or is this change safe? 🤔

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 saw the examples online too, but only in personal blog posts and not official Apple doc. It's already using a custom text field style WooRoundedBorderTextFieldStyle, which does use _body to customize the text field. The original POSSearchTextFieldStyle is more like a wrapper of the text field style to configure WooRoundedBorderTextFieldStyle and that's why I changed it to a view modifier. When it comes to WooRoundedBorderTextFieldStyle, I'd still keep the _body until there's a more official solution. Generally, I think APIs with _ prefix are not for official app usage and subject to change.

@jaclync
Copy link
Contributor Author

jaclync commented Oct 2, 2025

I remember I just rebuilt Periphery cache to avoid this but of course this is much better and beneficial when moving to POS module. We will be in much cleaner state.

Hah it depends on the scale of the warnings, if there had been more than 100 I'd have done the same to reset the Periphery cache.

@jaclync
Copy link
Contributor Author

jaclync commented Oct 2, 2025

@staskus Btw, if you tested barcode scanning on a physical iOS 26 tablet, did you notice that there was no way to recover the software keyboard unless the barcode scanner is disconnected? Similar to the previous Android issue pdfdoF-7J6-p2. Before iOS 26, I used to see a small software keyboard icon but in iOS 26 I didn't see any keyboard CTAs anymore.

@jaclync jaclync merged commit e06a16d into trunk Oct 2, 2025
15 checks passed
@jaclync jaclync deleted the feat/WOOMOB-935-periphery-warnings branch October 2, 2025 00:35
@staskus
Copy link
Contributor

staskus commented Oct 2, 2025

Before iOS 26, I used to see a small software keyboard icon, but in iOS 26 I didn't see any keyboard CTAs anymore.

@jaclync, it can be recovered, but a bit harder. You need to do that through the system top bar menu.

@jaclync
Copy link
Contributor Author

jaclync commented Oct 3, 2025

it can be recovered, but a bit harder. You need to do that through the system top bar menu.

Thanks for the tip! I found the "show keyboard" setting by swiping down the top bar.

IMG_0535

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: POS type: technical debt Represents or solves tech debt of the project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants