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 API and launcher for NonEditableBasicTextField #2395

Closed
wants to merge 1 commit into from

Conversation

oas004
Copy link
Contributor

@oas004 oas004 commented Sep 18, 2024

WHAT

Adding NonEditableBasicTextField composable to open the keyboard on click.

Currently a draft as I need to get it to work. Opening a draft PR here so I can get some feedback on the solution :)

WHY

Ref. #1563

HOW

Checklist 📋

  • Add explicit visibility modifier and explicit return types for public declarations
  • Run spotless check
  • Run tests
  • Update metalava's signature text files

import androidx.compose.ui.text.input.VisualTransformation

/**
* This is a wrapper for [BasicTextField] that when clicked launches the keyboard. It cannot be
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to improve these, but maybe wait until API is settled

@oas004 oas004 force-pushed the non-editable-basic-text-field branch from 5835a3c to fee27a5 Compare September 18, 2024 19:29

override fun parseResult(resultCode: Int, intent: Intent?): Result {
return if (resultCode == RESULT_OK) {
val resultText = intent?.extras?.getString("result_text")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This I need to fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it works, it's just that it's overridden by the BasicTextField implementation. Maybe using that wasn't such a good idea 🤔

@oas004 oas004 force-pushed the non-editable-basic-text-field branch from fee27a5 to 71ae7e0 Compare September 18, 2024 19:42
@oas004 oas004 force-pushed the non-editable-basic-text-field branch from 71ae7e0 to 2bd4ce7 Compare September 18, 2024 19:50
},
value = value,
onValueChange = {},
enabled = false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This made it work since its suppressing the BasicTextField, but it feels quite hacky, maybe not the solution we want to go for I guess 😅

Copy link
Member

Choose a reason for hiding this comment

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

What was not working before disabling the component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before doing this, I think the BasicTextField was interfering (just my assumption). The text was not able to update correctly

@yschimke yschimke requested a review from luizgrp September 19, 2024 15:47
Copy link
Member

@luizgrp luizgrp left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I will do another round of review tomorrow, just leaving these comments for now.

Could you please add a video showing the usage of the component to the PR description? A sequence of screenshots is also fine.

onTextChanged(it.text)
}
}
val keyboardIntent = Intent("com.google.android.wearable.action.LAUNCH_KEYBOARD")
Copy link
Member

Choose a reason for hiding this comment

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

This intent is for gboard, which doesn't come pre-installed in every watch (e.g. samsung watches).
This component would need to check if the intent exists, if it doesn't, we would need to think on a good behavior, e.g. call RemoteInputIntentHelper.startActivityForResult or let the user of the component provide the fallback behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I didn't know, maybe providing some callback would be good? No strong opinions on how best to handle this from my side. Is this maybe a UX question perhaps? :)

},
value = value,
onValueChange = {},
enabled = false,
Copy link
Member

Choose a reason for hiding this comment

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

What was not working before disabling the component?

@oas004
Copy link
Contributor Author

oas004 commented Sep 20, 2024

Thanks for working on this!

I will do another round of review tomorrow, just leaving these comments for now.

Could you please add a video showing the usage of the component to the PR description? A sequence of screenshots is also fine.

Thank you for checking this out and giving valuable feedback 🙏 Currently very in draft stage. I can provide a video on how it's working now. From what I understood in the issue we want to have a wrapper on BasicTextField exposing mostly the same API, that when clicking it it launches this intent instead to go to another screen to do the input. Is that the correct assumption? :)

@oas004
Copy link
Contributor Author

oas004 commented Sep 26, 2024

I think maybe I'll close this for now until we get a bit more input from the UX. Not quite sure how this component should work to provide most value to the users

@luizgrp
Copy link
Member

luizgrp commented Sep 26, 2024

Thanks for the patience, Odin. I will ping you here once I get more information.

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.

2 participants