Fix textoverflow on top appbar for components with long names #240

oas004 · 2022-07-07T20:00:59Z

Thank you for a very cool library! I absolutely love this! 😄 😍 When I started using it I noticed that the names of our components where quite long and that the toolbar was not quite handling this as you can see from the screenshots.

I had some trouble testing this and would love some help and feedback on that. I tried to make an Android test in the :showcase-browser-testsing but could not quite figure out a good way to test it. I also thought that it might be a bit flaky since the overflow will change with regards to screen size.

Before

Now

vinaygaba · 2022-07-08T04:11:41Z

@oas004 Could you upload the screenshots again. I'm not able to see them

vinaygaba · 2022-07-08T04:16:51Z

Re: Testing

You can add a composable here that has a really long value for name in the ShowkaseComposable or Preview annotation (a name that extends at least 2 lines). Then you can add a test case in this file that verifies that the entire name is visible. You are right about the different screen sizes generally speaking but the testing we are doing in this repo are restricted to a single device as defined in the android.yml file so it should be fine to add a test case.

vinaygaba · 2022-07-08T04:36:32Z

showkase/src/main/java/com/airbnb/android/showkase/ui/ShowkaseBrowserApp.kt

@@ -91,8 +92,7 @@ internal fun ShowkaseAppBar(
    Row(
        Modifier.fillMaxWidth()
            .graphicsLayer(shadowElevation = 4f)
-            .padding(padding2x)
-            .height(64.dp),
+            .padding(padding2x),


we might want to add a maxSize for the height but I'm curious to see the screenshots before that

hmm yeah maybe you're right 🤔 Is it also maybe a bit weird that it spans 3 lines instead of 2? What do you think? :)

oas004 · 2022-07-08T11:38:19Z

@oas004 Could you upload the screenshots again. I'm not able to see them

Hi, thanks for quick review! 😄 I tried to replace the screenshots now and upload them manually(instead of dragging and dropping them). Does it work now? :)

oas004 · 2022-07-08T13:04:46Z

Re: Testing

You can add a composable here that has a really long value for name in the ShowkaseComposable or Preview annotation (a name that extends at least 2 lines). Then you can add a test case in this file that verifies that the entire name is visible. You are right about the different screen sizes generally speaking but the testing we are doing in this repo are restricted to a single device as defined in the android.yml file so it should be fine to add a test case.

Thanks for quick review! Actually, when trying to write a test case for this I ran in to an issue. If I try to make a test on the node with test tag that the text is ellipsis, the NodeText has the entire text. So that means the text will go green if the text is correctly clipped an if It is not. There is an issue on this: https://issuetracker.google.com/issues/238332005 :) Could you please provide me with guidance on how to write a test case for this? :)

oas004 · 2022-07-10T00:14:30Z

showkase/src/main/java/com/airbnb/android/showkase/ui/ShowkaseBrowserApp.kt

@@ -186,19 +189,34 @@ private fun ShowkaseAppBarTitle(
    }
 }

+val LineCountKey = SemanticsPropertyKey<Int>("lineCount")


Should this be in its own file? :) What do u think?

Maybe move to a file called SemanticsUtils or something along those lines

Good idea! Moved it into SemanticsUtils in 833e234 😄

oas004 · 2022-07-10T00:17:13Z

Re: Testing
You can add a composable here that has a really long value for name in the ShowkaseComposable or Preview annotation (a name that extends at least 2 lines). Then you can add a test case in this file that verifies that the entire name is visible. You are right about the different screen sizes generally speaking but the testing we are doing in this repo are restricted to a single device as defined in the android.yml file so it should be fine to add a test case.

Thanks for quick review! Actually, when trying to write a test case for this I ran in to an issue. If I try to make a test on the node with test tag that the text is ellipsis, the NodeText has the entire text. So that means the text will go green if the text is correctly clipped an if It is not. There is an issue on this: https://issuetracker.google.com/issues/238332005 :) Could you please provide me with guidance on how to write a test case for this? :)

Managed to find a workaround for the test. Added it in baea74a 😄 What do you think?

vinaygaba · 2022-07-11T23:23:35Z

showkase/src/main/java/com/airbnb/android/showkase/ui/ShowkaseBrowserApp.kt

 @Composable
 fun ToolbarTitle(
    string: String,
    modifier: Modifier
 ) {
+    val lineCount = remember {


Not ideal to have test logic live in the component directly but I guess that happens pretty often in Compose due to how semantics are setup with some hooks into testing. Don't have a better idea on testing this so might be fine for now.

Totally agree with you on that! I think this can work for now :)

vinaygaba

Great work. One minor request and we are good to merge this after that.

Fix textoverflow on top appbar for components with long names

605db41

vinaygaba reviewed Jul 8, 2022

View reviewed changes

Add testcase for textoverflow in top app bar

baea74a

oas004 commented Jul 10, 2022

View reviewed changes

oas004 requested a review from vinaygaba July 11, 2022 23:12

vinaygaba reviewed Jul 11, 2022

View reviewed changes

vinaygaba approved these changes Jul 11, 2022

View reviewed changes

Move semantics to semantic utils

833e234

vinaygaba merged commit 504db33 into airbnb:master Jul 12, 2022

oas004 deleted the fix/textoverflow-top-appbar branch July 12, 2022 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix textoverflow on top appbar for components with long names #240

Fix textoverflow on top appbar for components with long names #240

oas004 commented Jul 7, 2022 •

edited

vinaygaba commented Jul 8, 2022

vinaygaba commented Jul 8, 2022

vinaygaba Jul 8, 2022

oas004 Jul 10, 2022

oas004 commented Jul 8, 2022

oas004 commented Jul 8, 2022

oas004 Jul 10, 2022

vinaygaba Jul 11, 2022

oas004 Jul 11, 2022

oas004 commented Jul 10, 2022

vinaygaba Jul 11, 2022

oas004 Jul 11, 2022

vinaygaba left a comment •

edited

Fix textoverflow on top appbar for components with long names #240

Fix textoverflow on top appbar for components with long names #240

Conversation

oas004 commented Jul 7, 2022 • edited

Before

Now

vinaygaba commented Jul 8, 2022

vinaygaba commented Jul 8, 2022

vinaygaba Jul 8, 2022

Choose a reason for hiding this comment

oas004 Jul 10, 2022

Choose a reason for hiding this comment

oas004 commented Jul 8, 2022

oas004 commented Jul 8, 2022

oas004 Jul 10, 2022

Choose a reason for hiding this comment

vinaygaba Jul 11, 2022

Choose a reason for hiding this comment

oas004 Jul 11, 2022

Choose a reason for hiding this comment

oas004 commented Jul 10, 2022

vinaygaba Jul 11, 2022

Choose a reason for hiding this comment

oas004 Jul 11, 2022

Choose a reason for hiding this comment

vinaygaba left a comment • edited

Choose a reason for hiding this comment

oas004 commented Jul 7, 2022 •

edited

vinaygaba left a comment •

edited