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

Add warning when typing in instance name #5506

Merged
merged 5 commits into from
Mar 23, 2023

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Mar 20, 2023

Closes #5489

A warning will appear when adding/editing an instance name (in the "Save as" field) at the end of a form.

What has been done to verify that this works as intended?

New tests and verified manually.

Why is this the best possible solution? Were any other approaches considered?

A few choices to discuss here:

  • The warning is shown as soon as the user taps on/focuses on the instance name field.
  • I opted to use a light grey instead of a light blue for our theme's 'Surface Variant' color (which is used as the background for Filled Cards). This was because it seemed more consistent with the Material 3 examples I looked at.
  • I moved the "Learn more..." out of the text (where it was as a link) and into a Text Button in the standard space for Card actions. Links in text are far harder to implement than distinct buttons, and I don't think what I've got for has any drawbacks (as far as I'm aware).
  • I decided to have the forum post open in the system browser in a new task. This way, the user can multitask or press back to return to Collect. I figured giving them options for returning to form entry while also keeping the relevant information up would be useful if they needed to then send that/show it to someone else.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Just adds the warning, so not a lot of risk here. This can probably merge without QA provided the reviewers have a quick play with it.

Before submitting this PR, please make sure you have:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@seadowg seadowg marked this pull request as ready for review March 21, 2023 15:09
@seadowg seadowg added the high priority Should be looked at before other PRs/issues label Mar 21, 2023
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

Works for me! Tests and code look good and I ran it.

If we end up using this component more we can iterate on the look and feel but given how few people will see this I think it's good enough. I probably would have gone with the blue background. I also do prefer the inline link over the button (but again, not important).

@dbemke
Copy link

dbemke commented Mar 23, 2023

Tested with Success!

Verified on Android 10

Verified Cases:

  • changing instance name in Fill Blank Form and Edit Saved Form
  • checking "Learn more” in the dialog
  • form with a generated instance name
  • rotating, minimizing, locking the screen
  • dark/light mode
  • don’t keep activities enabled/disabled

@srujner
Copy link

srujner commented Mar 23, 2023

Tested with Success!

Verified on Android 13

@seadowg seadowg merged commit cd78864 into getodk:master Mar 23, 2023
@seadowg seadowg deleted the instance-name-warning branch March 23, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior verified high priority Should be looked at before other PRs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show warning to users using manual instance name
5 participants