Skip to content
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

Open
wants to merge 6 commits into
base: v.next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
}
Comment on lines +20 to +25
Copy link
Member

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?


/// 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
Expand Up @@ -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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needed to change to -> FeatureFormView so that the two FeatureFormView modifiers can be placed in any order.

environment(\.validationErrorVisibility, visibility)
func validationErrors(_ visibility: ValidationErrorVisibility) -> FeatureFormView {
var copy = self
copy.validationErrorVisibility = visibility
return copy
}
}

Expand All @@ -39,6 +41,6 @@ extension EnvironmentValues {
}

/// The validation error visibility configuration of a form.
private struct FormViewValidationErrorVisibility: EnvironmentKey {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changing this to internal allows FeatureFormView to access the default value.

struct FormViewValidationErrorVisibility: EnvironmentKey {
static let defaultValue: FeatureFormView.ValidationErrorVisibility = .automatic
}
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ public struct FeatureFormView: View {
/// The title of the feature form view.
@State private var title = ""

/// The visibility of the form title.
var titleVisibility: TitleVisibility = .visible

/// The validation error visibility configuration of the form.
var validationErrorVisibility: ValidationErrorVisibility = FormViewValidationErrorVisibility.defaultValue

/// Initializes a form view.
/// - Parameters:
/// - featureForm: The feature form defining the editing experience.
Expand All @@ -94,7 +100,7 @@ public struct FeatureFormView: View {
ScrollViewReader { scrollViewProxy in
ScrollView {
VStack(alignment: .leading) {
if !title.isEmpty {
if !title.isEmpty && titleVisibility == .visible {
FormHeader(title: title)
Divider()
}
Expand All @@ -119,8 +125,9 @@ public struct FeatureFormView: View {
title = newTitle
}
}
.scrollDismissesKeyboard(.immediately)
.environment(\.validationErrorVisibility, validationErrorVisibility)
.environmentObject(model)
.scrollDismissesKeyboard(.immediately)
}
}

Expand Down
25 changes: 23 additions & 2 deletions Sources/ArcGISToolkit/Components/Popups/PopupView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not set on this modifier for one reason - the showCloseButton button modifier.

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)
Screenshot 2024-08-14 at 10 56 03 AM

Should we instead deprecate showCloseButton and add something like .popupHeader(_:) which would hide both?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 isPresented works if using the toolkit's FloatingPanel. And I guess it works with .sheet as well, but it's actually unnecessary with .sheet because we could have used the DismissAction environment value:

@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 header(_ visibility:).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

@mhdostal mhdostal Sep 13, 2024

Choose a reason for hiding this comment

The 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.
Expand Down