-
Notifications
You must be signed in to change notification settings - Fork 7
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
FeatureFormView
& PopupView
- Add ._Title(_:)
#839
base: v.next
Are you sure you want to change the base?
Changes from all commits
35a2d4b
66017eb
0f36ca3
cc848b4
de3b55d
d60f0ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// Copyright 2024 Esri | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// https://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
import SwiftUI | ||
|
||
public extension FeatureFormView { | ||
/// The visibility of the form title. | ||
/// - Since: 200.6 | ||
enum TitleVisibility: Sendable { | ||
/// The form title is hidden. | ||
case hidden | ||
/// The form title is visible. | ||
case visible | ||
} | ||
|
||
/// Specifies the visibility of the form title. | ||
/// - Parameter visibility: The preferred visibility of the form title. | ||
/// - Since: 200.6 | ||
func formTitle(_ visibility: TitleVisibility) -> FeatureFormView { | ||
var copy = self | ||
copy.titleVisibility = visibility | ||
return copy | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,8 +25,10 @@ public extension FeatureFormView { | |
|
||
/// Specifies the visibility of validation errors in the form. | ||
/// - Parameter visibility: The preferred visibility of validation errors in the form. | ||
func validationErrors(_ visibility: ValidationErrorVisibility) -> some View { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needed to change to |
||
environment(\.validationErrorVisibility, visibility) | ||
func validationErrors(_ visibility: ValidationErrorVisibility) -> FeatureFormView { | ||
var copy = self | ||
copy.validationErrorVisibility = visibility | ||
return copy | ||
} | ||
} | ||
|
||
|
@@ -39,6 +41,6 @@ extension EnvironmentValues { | |
} | ||
|
||
/// The validation error visibility configuration of a form. | ||
private struct FormViewValidationErrorVisibility: EnvironmentKey { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing this to internal allows |
||
struct FormViewValidationErrorVisibility: EnvironmentKey { | ||
static let defaultValue: FeatureFormView.ValidationErrorVisibility = .automatic | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,16 +71,19 @@ public struct PopupView: View { | |
/// so that the the "close" button can close the view. | ||
private var showCloseButton = false | ||
|
||
/// The visibility of the popup title. | ||
private var titleVisibility: TitleVisibility = .visible | ||
|
||
/// The result of evaluating the popup expressions. | ||
@State private var evaluation: Evaluation? | ||
|
||
/// A binding to a Boolean value that determines whether the view is presented. | ||
private var isPresented: Binding<Bool>? | ||
|
||
public var body: some View { | ||
VStack(alignment: .leading) { | ||
HStack { | ||
if !popup.title.isEmpty { | ||
if !popup.title.isEmpty && titleVisibility == .visible { | ||
Text(popup.title) | ||
.font(.title) | ||
.fontWeight(.bold) | ||
|
@@ -203,6 +206,24 @@ extension PopupView { | |
} | ||
|
||
extension PopupView { | ||
/// The visibility of the popup title. | ||
/// - Since: 200.6 | ||
public enum TitleVisibility: Sendable { | ||
/// The popup title is hidden. | ||
case hidden | ||
/// The popup title is visible. | ||
case visible | ||
} | ||
|
||
/// Specifies the visibility of the popup title. | ||
/// - Parameter visibility: The visibility of the popup title. | ||
/// - Since: 200.6 | ||
public func popupTitle(_ visibility: TitleVisibility) -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not set on this modifier for one reason - the It seems disjoint that a developer can hide the title and simultaneously leave the close button visible. PopupView(popup: popup!, isPresented: $showPopup)
.popupTitle(.hidden)
.showCloseButton(true) Should we instead deprecate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The title and close button are not necessarily related, other than they have been occupying the same space. It may be that the client doesn't want the title in the popup/view but still requires the close button. So I'm OK having them both be distinct and put the onus on the client to figure out how they want it to display/behave. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with David that it is a bit disjoint. I'm not sure we should've had a simple boolean for the close button. Some people might prefer a "Done" button. There is no way to customize that (well I guess the library is open source so people could customize it). The @Environment(\.dismiss) private var dismiss I think, for the most part, our components should not assume their container. Since title text is sometimes (often) on the "same line" as some buttons (done, cancel, menu), we might do better with a constructor like this that enables the PopupView to build a header for situations where they want to add a close button, but the title isn't displayed in a navigation toolbar: struct PopupView<HeaderContent: View>: View {
let popup: Popup
let headerContent: ((String) -> HeaderContent)?
...
/// Creates a PopupView...
/// - Parameters:
/// - popup: The Popup to display
/// - headerContent: A closure that builds the header for a given title
init(popup: Popup, @ViewBuilder headerContent: @escaping (String) -> HeaderContent) {
self.popup = popup
self.headerContent = headerContent
}
This would allow somebody to add their own title, buttons, etc. That would solve the close button thing. Then I guess there could be some modifier for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mhdostal Any preference on the individual title and close button controls vs. Ryan's suggestion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm OK with @rolson 's suggestion. That does solve the issue with the "close" button and allows clients to set it up how they want. It's a good point that our components should not assume their container. We could be better trying different containers to verify that. |
||
var copy = self | ||
copy.titleVisibility = visibility | ||
return copy | ||
} | ||
|
||
/// Specifies whether a "close" button should be shown to the right of the popup title. If the "close" | ||
/// button is shown, you should pass in the `isPresented` argument to the `PopupView` | ||
/// initializer, so that the the "close" button can close the 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.
Why an enum instead of a
Bool
?shouldShowTitle
, or something similar?