-
Notifications
You must be signed in to change notification settings - Fork 31
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
DON-739 - Update Search Input Summary based on new designs #2078
Conversation
cb334f8
to
7fc3f7d
Compare
@@ -86,6 +86,7 @@ internal fun BpkTextFieldImpl( | |||
minLines: Int = 1, | |||
maxLines: Int = 1, | |||
interactionSource: MutableInteractionSource = remember { MutableInteractionSource() }, | |||
isFocused: Boolean? = 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.
Why do we need to add this field? Can we always use the mutable interaction source instead?
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 found it caused issues with the screen reader when changing the focus state programatically.
Before
date_selector_focus_requesters.mp4
After
date_selector_focus_property.mp4
placeholder = inputHint, | ||
prefix = prefix, | ||
status = BpkFieldStatus.Default, | ||
clearAction = clearAction, | ||
type = BpkTextFieldType.Search, | ||
isFocused = type is BpkSearchInputSummaryType.ReadOnly && type.isFocused, |
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.
small thing here: I think it'd be better to change this condition to only set a non-null value for the ReadOnly
type as the default input one should use the system focused state
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.
Yes, that makes sense. I've updated 👍
Support clear action when read only in BpkSearchInputSummary
7fc3f7d
to
e94a353
Compare
Based on the new designs for the Date Selector, we need to make some slight changes to the Search Input Summary component.
Support there being no prefix and allowing for it to be read only.
I additionally had to make a modification deeper in
BpkTextFieldImpl
so that the clear button will appear even though it is read only (this is why there is theNone
prefix rather than it being optional).I added the new case to the bottom of the screen in the app, see below.
Remember to include the following changes:
README.md
If you are curious about how we review, please read through the code review guidelines