Closed nick0602 closed 4 years ago
Hey there! Thank you for raising this issue and reaching out. I think you solution works fine, with one exception: instead of making nullable builder for up indicator, it's better to have default value and avoid nullables at all. I personally like the direction you're taking with solving this issue, but let's also see what @Vacxe is thinking.
Hey, thanks for the feedback!
I thought about the non-nullability of navigationIcon
, but at first I had one huge doubt regarding it: since we have to pass a contentDescription
- in the beginning I thought as plain string - it's not as "strong" (or reliable) as using ids, mainly because the default contentDescription
might not be Navigate up
. I checked by changing language and it depends on phone locale, for example in Italian it's Torna indietro
.
There might be cases where the phone language is not English in some tests, so I don't know if that might cause some confusion?
I took a look at Toolbar.java (line 325) and the contentDescription
is fetched like this:
final CharSequence navDesc = a.getText(R.styleable.Toolbar_navigationContentDescription);
if (!TextUtils.isEmpty(navDesc)) {
setNavigationContentDescription(navDesc);
}
It didn't really look promising.
However, I just took a look at the string values provided by androidx.appcompat
and there is
R.string.abc_action_bar_up_description
which is actually Navigate up
/Torna indietro
[...].
What if we used
withContentDescription(androidx.appcompat.R.string.abc_toolbar_collapse_description)
as default matcher? (maybe importing androidx.appcompat.R
as something shorter)
Toolbar.kt
would become:
class KToolbar : KBaseView<KToolbar>, ToolbarViewActions, ToolbarViewAssertions {
override val navigationIcon: KImageView // no more nullable
constructor(
builder: ViewBuilder.() -> Unit,
homeAsUpIndicatorBuilder: (ViewBuilder.() -> Unit) = { withContentDescription(androidx.appcompat.R.string.abc_action_bar_up_description)}
) : super(builder) {
navigationIcon = KImageView(homeAsUpIndicatorBuilder)
}
constructor(
parent: Matcher<View>,
builder: ViewBuilder.() -> Unit,
homeAsUpIndicatorBuilder: (ViewBuilder.() -> Unit) = { withContentDescription(androidx.appcompat.R.string.abc_action_bar_up_description)}
) : super(parent, builder) {
navigationIcon = KImageView(homeAsUpIndicatorBuilder)
}
constructor(
parent: DataInteraction,
builder: ViewBuilder.() -> Unit,
homeAsUpIndicatorBuilder: (ViewBuilder.() -> Unit) = { withContentDescription(androidx.appcompat.R.string.abc_action_bar_up_description)}
) : super(parent, builder) {
navigationIcon = KImageView(homeAsUpIndicatorBuilder)
}
}
I need to test it a bit more thoroughly later today but this way we could get rid of the nullable and also not have to worry about locale.
On a side note: in my current implementation of hasHomeAsUpIndicator
I'm relying on a custom matcher along the lines of what has been done for hasTitle
and hasSubtitle
, but I'm considering dropping it and use navigationIcon.isVisible()
instead to keep it consistent.
@nick0602 Thanks for rising a ticket! @Unlimity Ill check it in couple hours.
Probably if navi buttons can be matched by KImageView { withContentDescription("Navigate up") }
we can decalare internal variable inside KToolBar
for avoinig matching delegate in KToolBar
constructor
@Vacxe If you wanna take a look last night I tried withContentDescription
with the appcompat string on another branch removing its nullability: https://github.com/agoda-com/Kakao/compare/master...nick0602:feature/toolbar-navigation-button.
As for the KImageView
yeah, I did declare it in the interface but it can be moved down to KToolbar
directly and ToolbarViewAssertions
only have the contracts for hasHomeAsUpIndicator
and hasHomeAsUpIndicatorDrawable
rather than default implementations, but the param in the constructor is still needed as the icon could have any custom description set though (I added a test for that on my branch as well).
Please check solution on different languages? I think it will work only with English locale.
@hram androidx.appcompat.R.string.abc_action_bar_up_description
is a localized string with translations, I tested w/ italian (Torna indietro
) and spanish (Navegar hacia arriba
) and it works.
I checked in debug while running tests as well and watching
InstrumentationRegistry.getInstrumentation().targetContext.getString(androidx.appcompat.R.string.abc_action_bar_up_description)
I get a different translated string based on the locale.
@nick0602 I mean that you can't use "Navigate up" in solution. This will work only in English locale.
Hey @Vacxe, did you get a chance to take a look at this? 😃
@nick0602 sorry about that, just I a bit overloaded for this time. I will look ASAP.
@nick0602 @hram @Unlimity
Hi, I've been using Kakao for a few weeks now and I noticed that recently you added ActionBar support (#188). However, I saw that currently there's no support to match the up indicator drawable.
At my company, in our current UI tests we're basically declaring two different
KView
s as a workaround, aKToolbar
and aKImageView
for the up indicator button:and then perform the checks with
However I'd like to know if it's possible to have an implementation of
KToolbar
that allows what we're trying to achieve.I forked the repo and tried to play around a bit (please ignore #215 as I was comparing and accidentally opened the PR) and everything seems to be working fine, the changes I had in mind were:
KToolbar
constructor that helps locate thehomeAsUpIndicator
via itscontentDescription
(by default it should be either "Navigate up"/"Navigate home", customizable viasetHomeActionContentDescription
) - and define it asKImageView
already (?)hasDrawable
on it by passing the expected drawable of the indicatorIn case if
hasNavigationIconDrawable
is invoked but there's no view builder provided, it throws anAssertionError
.For just checking if the homeAsUpIndicator is present there could be another method (e.g.
hasHomeAsUpIndicator
) as well to allow checking if it's just visible when a custom icon is not set.Note that I haven't been able to find the "default" back indicator drawable ID for matching it so I was planning to add a drawable to the sample project and set it programmatically.
I was also trying to see if the
icon
could be checked as well (the one set viasetIcon
) but I couldn't find anything that allowed to match it properly.What do you think about it? I'd like to discuss this with you and see if it can be implemented, in that case I'll rework the branch I have on my fork with an actual decent commit history and submit a (new) PR.
Thanks!
EDIT: I did some changes and pushed a new branch, it's here if you want to take a look.