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 ability to hide default headers #839

Open
wants to merge 18 commits into
base: v.next
Choose a base branch
from

Conversation

dfeinzimer
Copy link
Collaborator

@dfeinzimer dfeinzimer commented Aug 14, 2024

Closes #835

  • Adds:
    • PopupView.popupHeader(_:)
      • Controls the visibility of the title and close button (visible by default).
    • FeatureFormView.formHeader(_:)
      • Controls the visibility of the title (visible by default).
  • Deprecates:
    • PopupView.showCloseButton(_:)

@dfeinzimer dfeinzimer self-assigned this Aug 14, 2024
@@ -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.

@@ -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

@dfeinzimer dfeinzimer Aug 14, 2024

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 return Self so that the two FeatureFormView modifiers can be placed in any order.

Sources/ArcGISToolkit/Components/Popups/PopupView.swift Outdated Show resolved Hide resolved
@dfeinzimer dfeinzimer marked this pull request as ready for review August 14, 2024 17:58
@dfeinzimer dfeinzimer removed the request for review from rolson October 17, 2024 23:35
@dfeinzimer dfeinzimer marked this pull request as draft October 17, 2024 23:48
@dfeinzimer dfeinzimer changed the title FeatureFormView & PopupView - Add ._Title(_:) FeatureFormView & PopupView - Add ability to hide default headers Nov 18, 2024
@@ -95,7 +101,7 @@ public struct FeatureFormView: View {
ScrollViewReader { scrollViewProxy in
ScrollView {
VStack(alignment: .leading) {
if !title.isEmpty {
if !title.isEmpty && headerVisibility != .hidden {
FormHeader(title: title)
Divider()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Divider is included as part of the header so that a developer can control whether or not it's included.

Same behavior with the new PopupView modifier.

}
Divider()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Divider is included as part of the header so that a developer can control whether or not it's included.

Same behavior with the new FeatureFormView modifier.

@@ -67,7 +67,10 @@ public struct PopupView: View {
/// A Boolean value specifying whether a "close" button should be shown or not. If the "close"
/// button is shown, you should pass in the `isPresented` argument to the initializer,
/// so that the the "close" button can close the view.
private var showCloseButton = false
var showCloseButton = false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

showCloseButton is now internal so that it can be modified from PopupView+ShowCloseButton.swift.

Comment on lines -205 to -217
extension PopupView {
/// 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.
/// Defaults to `false`.
/// - Parameter newShowCloseButton: The new value.
/// - Returns: A new `PopupView`.
public func showCloseButton(_ newShowCloseButton: Bool) -> Self {
var copy = self
copy.showCloseButton = newShowCloseButton
return copy
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to PopupView+ShowCloseButton.swift to be consistent with how other PopupView and FeatureFormView modifiers are written.

@dfeinzimer
Copy link
Collaborator Author

@mhdostal and @rolson this is ready for review once again. The updated changes are:

  • Adds:
    • PopupView.popupHeader(_:)
      • Controls the visibility of the title and close button (visible by default).
    • FeatureFormView.formHeader(_:)
      • Controls the visibility of the title (visible by default).
  • Deprecates:
    • PopupView.showCloseButton(_:)

In regards to the suggestion above, would it be better to avoid the complication of maintaining new initializers? With these changes devs can hide the header and easily include a custom one with a VStack:

.sheet(isPresented: $showPopup) { [popup] in
    VStack(alignment: .leading) {
        Text(popup!.title)
            .font(.title)
        PopupView(popup: popup!)
            .popupHeader(.hidden)
    }
    .padding()
}

@dfeinzimer dfeinzimer marked this pull request as ready for review November 19, 2024 00:52
@dfeinzimer
Copy link
Collaborator Author

This is still ready for review. I made one more slight change to avoid breaking the behavior of PopupView.showCloseButton(_:).

Copy link
Member

@mhdostal mhdostal left a comment

Choose a reason for hiding this comment

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

One question.

.labelStyle(.iconOnly)
Spacer()
if (showCloseButtonDeprecatedModifierIsApplied && showCloseButton)
|| (!showCloseButtonDeprecatedModifierIsApplied && isPresented != nil) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it assumed that if isPresented != nil then isPresented?.wrappedValue == true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, assumed. One could technically do something odd like pass .constant(false) into the initializer parameter and still have the PopupView presented but that would be unintended use of the parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants