-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Compose: Add DaxTextField #7247
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
base: develop
Are you sure you want to change the base?
Conversation
...design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxSecureTextField.kt
Show resolved
Hide resolved
...ystem/design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxTextField.kt
Show resolved
Hide resolved
...ystem/design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxTextField.kt
Show resolved
Hide resolved
malmstein
left a comment
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.
very nice work, just a couple of nits. I’ll leave it to @mikescamell for the final approval.
...id-design-system/design-system/src/main/java/com/duckduckgo/common/ui/compose/theme/Theme.kt
Outdated
Show resolved
Hide resolved
...ystem/design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxTextField.kt
Show resolved
Hide resolved
...ystem/design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxTextField.kt
Outdated
Show resolved
Hide resolved
...ystem/design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxTextField.kt
Outdated
Show resolved
Hide resolved
...ystem/design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxTextField.kt
Outdated
Show resolved
Hide resolved
...ystem/design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxTextField.kt
Outdated
Show resolved
Hide resolved
...ystem/design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxTextField.kt
Show resolved
Hide resolved
...id-design-system/design-system/src/main/java/com/duckduckgo/common/ui/compose/theme/Theme.kt
Outdated
Show resolved
Hide resolved
...id-design-system/design-system/src/main/java/com/duckduckgo/common/ui/compose/theme/Color.kt
Outdated
Show resolved
Hide resolved
...ystem/design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxTextField.kt
Outdated
Show resolved
Hide resolved
...ystem/design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxTextField.kt
Outdated
Show resolved
Hide resolved
...design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxSecureTextField.kt
Outdated
Show resolved
Hide resolved
@landomen I can still click on the disabled version and get focus. |
...ystem/design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxTextField.kt
Outdated
Show resolved
Hide resolved
I think we need yo end up in a state of For it not to be focusable |
Thanks for catching this! This was a bug because of the wrong combination of arguments after I cleaned up the code 😅. It's fixed now with the new enum. |
89c6858 to
b3b471e
Compare
|
@mikescamell @GerardPaligot Thanks for all your great feedback! I've refactored the composable, please take another look when you get a chance. |
.../java/com/duckduckgo/common/ui/internal/ui/component/textinput/ComponentTextInputFragment.kt
Show resolved
Hide resolved
...design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxSecureTextField.kt
Show resolved
Hide resolved
GerardPaligot
left a comment
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 new version is really good!
| } | ||
|
|
||
| // Non-editable text full click listener with end icon | ||
| view.setupThemedComposeView(id = com.duckduckgo.common.ui.internal.R.id.compose_text_input_30, isDarkTheme = isDarkTheme) { |
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.
We have a difference between the XML and Compose components here.
When I'm clicking on the XML view, the view isn't focused.
The same thing on the Compose component switch the border in blue (focus mode).
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.
Not sure if there is an easy way to prevent that. Would probably have to do something with the MutableInteractionSource but that could be tricky. In my opinion, the current behavior is okay, because you interacted with the text field (it's still enabled).
The XML one sometimes also behaves like that, at least on the "Non-editable text with line truncation" example. And there is a bug on the XML one where the keyboard still shows up and you can enter new text even though its set to non-editable 😅
.../java/com/duckduckgo/common/ui/internal/ui/component/textinput/ComponentTextInputFragment.kt
Show resolved
Hide resolved
...design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxSecureTextField.kt
Show resolved
Hide resolved
| // Editable password that fits in one line | ||
| view.setupThemedComposeView(id = com.duckduckgo.common.ui.internal.R.id.compose_text_input_6, isDarkTheme = isDarkTheme) { | ||
| val state = rememberTextFieldState("Loremipsumolor") | ||
| DaxSecureTextField( |
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.
We have a difference between the XML and Compose components here.
When I'm clicking on the XML view, the content is no more masked.
When I'm clicking on the Compose view, the content stay masked.
From my opinion, we should keep the Compose behavior but because there is a difference, I want to report it here.
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.
Thanks for reporting it!
Yeah, the XML one first shows unmasked text when focused and masking it requires action.
The Compose one first shows masked text when focused and unmasking it requires action.
I agree the Compose behavior is better, more secure and I think more in line with how a secure text input should behave. So I'm voting for keeping it. @mikescamell Your thoughts?
| } | ||
|
|
||
| // Error | ||
| view.setupThemedComposeView(id = com.duckduckgo.common.ui.internal.R.id.compose_text_input_21, isDarkTheme = isDarkTheme) { |
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.
We have a difference between the XML and Compose components here.
When I'm clicking on the XML view, the cursor is white/black (according to if you are in dark/light mode).
When I'm clicking on the Compose view, the cursor is in red.
From my opinion, we should keep the Compose behavior but because there is a difference, I want to report it here.
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 catch! I also vote for the Compose behavior, but making the cursor color match XML should be simple. @mikescamell How do you feel?
| ) { | ||
| // needed by the OutlinedTextField container | ||
| val internalInteractionSource = interactionSource ?: remember { MutableInteractionSource() } | ||
| var isPasswordVisible by remember { mutableStateOf(false) } |
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.
Suggestion: What do you think to put a boolean parameter in the compose contract and let the component consumer control the visibility state of this component? This will allow us to keep a stateless component and give a better control of the component to the consumer. If we want to display the password for any other reasons than a click on the trailing icon, we can't with our actual contract.
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.
Lifted the state outside so it's now the consumers responsibility. It makes sense to have a stateless component, however I do think this impacts the ease of use by needing more boilerplate state management.
In all the currently supported use-cases , the only reason to show/hide password is because of the internal icon click. It seems we're predicting potential other usages in the future, incorporating that in the API, and by doing that, making it harder to use for existing purpose.
...design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxSecureTextField.kt
Outdated
Show resolved
Hide resolved
...design-system/src/main/java/com/duckduckgo/common/ui/compose/textfield/DaxSecureTextField.kt
Outdated
Show resolved
Hide resolved
lint-rules/src/main/java/com/duckduckgo/lint/ui/DaxTextFieldTrailingIconDetector.kt
Show resolved
Hide resolved
464f7a6 to
a7aa6a2
Compare
| Icon( | ||
| painter = painter, | ||
| contentDescription = contentDescription, | ||
| tint = DuckDuckGoTheme.iconColors.primary, |
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.
Bug: Trailing icon ignores error state color configuration
The DaxTextFieldTrailingIcon hardcodes tint = DuckDuckGoTheme.iconColors.primary, which overrides the Material3 LocalContentColor mechanism. This means the errorTrailingIconColor = DuckDuckGoTheme.colors.destructive configured in daxTextFieldColors() is never applied - when a text field is in error state, the border and label turn red but the trailing icon stays in its normal primary color instead of changing to destructive. Using LocalContentColor.current or Color.Unspecified for the tint would allow the icon to properly reflect error state.
Please tell me if this was useful or not with a 👍 or 👎.


Task/Issue URL: https://app.asana.com/1/137249556945/project/1210594645151737/task/1211659112661196?focus=true
Description
Adds a Compose version of the DaxTextField.
Steps to test this PR
ADS Preview
UI changes
See ADS Preview screen
Note
Introduces Compose text input components (text and secure), integrates them into ADS preview, updates theme colors, and adds lint rules enforcing trailing icon usage.
DaxTextFieldandDaxSecureTextFieldadded with label, error, input modes (editable/read-only/disabled), line limits, keyboard options, input transformation, and trailing icon API viaDaxTextFieldTrailingIconScope.DaxSecureTextFieldand combinable with custom trailing icon.DuckDuckGoColorswithtextField.bordersandicons.primary; addWhite78.DuckDuckGoTheme(light/dark palettes) and exposeiconColors.ComponentTextInputFragmentandcomponent_text_input_view.xmlupdated to showcase Compose variants: single/multi/form, read-only with/without icons, passwords (incl. visibility), error/disabled, IP/URL inputs, observable text, auto-select on focus, and programmatic focus.DaxTextFieldTrailingIconDetectorandDaxSecureTextFieldTrailingIconDetector; register inDuckDuckGoIssueRegistrywith comprehensive tests.Written by Cursor Bugbot for commit a7aa6a2. This will update automatically on new commits. Configure here.