-
Notifications
You must be signed in to change notification settings - Fork 6
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
FeatureForm
: support for UtilityAssociations
#776
base: v.next
Are you sure you want to change the base?
Conversation
@@ -23,6 +23,7 @@ plugins { | |||
id("artifact-deploy") | |||
id("com.google.android.libraries.mapsplatform.secrets-gradle-plugin") | |||
alias(libs.plugins.binary.compatibility.validator) apply true | |||
alias(libs.plugins.kotlin.serialization) apply true |
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.
Serialization plugin is needed for compose navigation. Specifically, for the route classes used.
@@ -99,7 +98,7 @@ class DateTimeFieldTests : FeatureFormTestRunner( | |||
} | |||
dateTimeField.assertIsDisplayed() | |||
val helper = dateTimeField.onChildWithContentDescription("supporting text") | |||
val helperMatcher = hasText("Date Entry is Required") | |||
val helperMatcher = hasText("Required") |
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.
Updated the correct text to check for.
evaluateExpressions() | ||
} | ||
|
||
internal constructor( |
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.
This constructor is to primarily support the deprecated FeatureForm
composable which creates its own FormStateCollection
.
form: FeatureForm, | ||
elements: List<FormElement>, | ||
scope: CoroutineScope, | ||
ignoreList : Set<Class<out FormElement>> = emptySet(), |
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.
This param allows the deprecated FeatureForm
composable to not display any FormElement
s such as UtillityAssociationsFormElement
.
*/ | ||
@Composable | ||
public fun FeatureForm( | ||
featureForm: FeatureForm, | ||
featureFormState: FeatureFormState, | ||
onDismiss: (() -> Unit)?, |
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.
Should this use a different mechanism to hide the close icon? For ex, use a showCloseIcon: Boolean
instead of an optional parameter.
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.
use a showCloseIcon: Boolean instead of an optional parameter.
I think that would be a good replacement
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.
Updated!
@@ -159,8 +149,6 @@ internal class AttachmentElementState( | |||
state.loadWithParentScope() | |||
// scroll to the new attachment after a delay to allow the recomposition to complete | |||
scope.launch { | |||
delay(100) | |||
lazyListState.scrollToItem(0) |
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.
Moved this into the composition as I was seeing an exception about doing this scrollToItem
during the measurement phase.
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.
Some initial observations/suggestions
...reFormsApp/app/src/main/java/com/arcgismaps/toolkit/featureformsapp/screens/map/MapScreen.kt
Show resolved
Hide resolved
...reFormsApp/app/src/main/java/com/arcgismaps/toolkit/featureformsapp/screens/map/MapScreen.kt
Show resolved
Hide resolved
} | ||
) | ||
} | ||
|
||
// show pick a feature dialog if the layer is a sublayer | ||
is UIState.SelectFeature -> { |
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.
could this be a navigation route?
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.
Possibly. I think that would be a good idea. It would also mean moving other screens like the FeatureFormSheet
and AddFeatureSheet
into separate destinations. If its okay, I will tackle this in a separate PR.
...reFormsApp/app/src/main/java/com/arcgismaps/toolkit/featureformsapp/screens/map/MapScreen.kt
Outdated
Show resolved
Hide resolved
...reFormsApp/app/src/main/java/com/arcgismaps/toolkit/featureformsapp/screens/map/MapScreen.kt
Outdated
Show resolved
Hide resolved
...ormsApp/app/src/main/java/com/arcgismaps/toolkit/featureformsapp/screens/map/MapViewModel.kt
Outdated
Show resolved
Hide resolved
...ormsApp/app/src/main/java/com/arcgismaps/toolkit/featureformsapp/screens/map/MapViewModel.kt
Outdated
Show resolved
Hide resolved
...ormsApp/app/src/main/java/com/arcgismaps/toolkit/featureformsapp/screens/map/MapViewModel.kt
Outdated
Show resolved
Hide resolved
...ormsApp/app/src/main/java/com/arcgismaps/toolkit/featureformsapp/screens/map/MapViewModel.kt
Outdated
Show resolved
Hide resolved
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.
a few more comments
toolkit/featureforms/src/main/java/com/arcgismaps/toolkit/featureforms/internal/utils/Dialog.kt
Outdated
Show resolved
Hide resolved
...ismaps/toolkit/featureforms/internal/components/utilitynetwork/UtilityAssociationsElement.kt
Show resolved
Hide resolved
.../java/com/arcgismaps/toolkit/featureforms/internal/components/utilitynetwork/Associations.kt
Outdated
Show resolved
Hide resolved
.../java/com/arcgismaps/toolkit/featureforms/internal/components/utilitynetwork/Associations.kt
Outdated
Show resolved
Hide resolved
.../java/com/arcgismaps/toolkit/featureforms/internal/components/utilitynetwork/Associations.kt
Outdated
Show resolved
Hide resolved
.../java/com/arcgismaps/toolkit/featureforms/internal/components/utilitynetwork/Associations.kt
Outdated
Show resolved
Hide resolved
toolkit/featureforms/src/main/java/com/arcgismaps/toolkit/featureforms/FeatureFormState.kt
Outdated
Show resolved
Hide resolved
toolkit/featureforms/src/main/java/com/arcgismaps/toolkit/featureforms/FeatureFormState.kt
Show resolved
Hide resolved
DisposableEffect(state) { | ||
onDispose { | ||
state.setNavigationCallback(null) | ||
state.setNavigateBack(null) | ||
} |
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.
Added DisposableEffect
to set the navigation events back to null.
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.
Good work, a few more things.
toolkit/featureforms/src/main/java/com/arcgismaps/toolkit/featureforms/FeatureFormState.kt
Outdated
Show resolved
Hide resolved
* Pops the current [FeatureForm] from the stack and sets the previous form as the [activeFeatureForm]. | ||
*/ | ||
internal fun popBackStack(): Boolean { | ||
return if (store.size <= 1) { |
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.
return if (store.size <= 1) { | |
return if (store.isEmpty()) { |
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.
It's actually meant to check there is more than 1 element in the stack.
toolkit/featureforms/src/main/java/com/arcgismaps/toolkit/featureforms/FeatureFormState.kt
Show resolved
Hide resolved
toolkit/featureforms/src/main/java/com/arcgismaps/toolkit/featureforms/FeatureFormState.kt
Show resolved
Hide resolved
toolkit/featureforms/src/main/java/com/arcgismaps/toolkit/featureforms/FeatureFormState.kt
Outdated
Show resolved
Hide resolved
toolkit/featureforms/src/main/java/com/arcgismaps/toolkit/featureforms/FeatureForm.kt
Outdated
Show resolved
Hide resolved
toolkit/featureforms/src/main/java/com/arcgismaps/toolkit/featureforms/FeatureForm.kt
Outdated
Show resolved
Hide resolved
also updated api dump
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.
Great work @kaushikrw 👍🏼
Related to issue: #kotlin/5456
Description:
Adds UtilityAssociations support for
FeatureForm
.UtilityAssociations UI structure
UtilityAssociationsFormElement
is displayed if it is part of the form info.element.fetchAssociationsFilterResults
which fetches the list of associations organized within a specific structure represented by theUtilityAssociationsFilterResult
class.UtilityAssociationsFilter
s.UtilityAssociations
that are grouped by layer, sublayer, table or subtable. This is represented by theUtilityAssociationGroupResult
class.UtilityAssociation
s present in theUtilityAssociationGroupResult
.Navigation Support
FeatureForm
of anArcGISFeature
on the other end of theUtilityAssociation
. This is achieved by adding a navigation graph within the component.Serializable
orParcelable
objects can be saved, for example during configuration changes, this means theFeatureForm
api object cannot be used here and has to hoisted out of the navigation graph.Stack
data structure is created out of the composition that will track all theFeatureForm
s visited. This stack will closely track the current navigation stack and provide the source of truth for the navigation destinations.API Changes
Since the
Stack
has to be out of the composition, this means providing a State holder class for theFeatureForm
composable. This state holder can then be hoisted up, like in aViewModel
. Hence the proposed API changes -FeatureFormState
class which internally creates and holds on to thisStack
ofFeatureForm
api objects.activeFeatureForm
property that will always point to the currentFeatureForm
instance being edited.FeatureForm
composable in favor of the new one.FeatureForm
composable utilizes theFeatureFormState
.UtilityAssociationsFormElement
s and will function and look exactly like it used to.ValidationErrorVisibility
class as the updated workflow specifies to show all errors when the form is opened. This setting will possibly be part of the web map spec in a future release.FeatureForm
composable provides an optionalonDimiss
callback. If this is provided, a "close" icon is displayed which when clicked raises this event.FeatureForm
composable provides a booleanshowFormActions
. If this is true (default), A "Discard" and "Save" buttons within an action bar is displayed when there are edits in the form.FeatureFormEditingEvent
is introduced which is raised when the user "Saves" or "Discards" edits on the Form. (see ui changes below).UI Changes
FeatureForm.hasEdits
property is used to show an action bar on top of the form ifshowFormActions
is true which enables two options -FeatureForm.finishEditing()
and raises theFeatureFormEditingEvent.SavedEdits
if only the result of the operation is successful.FeatureForm.discarEdits()
and raises theFeatureFormEditingEvent.DiscardedEdits
.onDimiss
callback is provided, a close icon is shown. If there are active edits on the form as indicated byFeatureForm.hasEdits
, a dialog is presented to save or discard edits. These actions will have the same behavior as above.Feature
, the same workflow applies. If there are edits, a dialog is presented to save/discard.FeatureForm.validationErrors
, a dialog is presented with this information.These changes should satisfy the workflows mentioned in the issue here.
Internal Changes
FormElement
s are now stored in theFeatureFormState
class, this means we no longer need anyrememberSaveable
methods on these elements.Test Changes
Test Data
I've added some test data in the linked issue.
Notes
Pre-merge Checklist