-
Notifications
You must be signed in to change notification settings - Fork 108
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
[Woo POS] Add simple-only product banner to dashboard #13319
Conversation
|
@@ -37,6 +34,63 @@ struct ItemListView: View { | |||
/// View Helpers | |||
/// | |||
private extension ItemListView { | |||
@ViewBuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on using ViewBuilder
vs computed variables for the views? I tend to use the first one if there's some conditional logic or we need to pass params, but doesn't seem to be needed for all cases 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can put @ViewBuilder
for both functions and computed variables
} | ||
VStack { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Updated dc3bc34
.background(Color.posBackgroundWhitei3) | ||
} | ||
|
||
var headertextView: some View { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: headerTextView
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: f62db85
var headertextView: some View { | ||
Text(Localization.productSelectorTitle) | ||
.frame(maxWidth: .infinity, alignment: .leading) | ||
.padding(.vertical, 8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can also have this 8
in Constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Moved on 196451f
Closes: #13317
Description
This PR adds a toggeable banner to the header of the item list view, in order to the current POS constraint for simple products only.
Please note the UI is not final yet, so we haven't addressed the fine-tuned details, just the overall view and its behaviour. In future PRs we also want to tie the banner display to once per app-install, at the moment will just appear all the time by default on init.
Testing
x
icon and observe it collapses. Tap thei
icon and observe it expands again.