-
Notifications
You must be signed in to change notification settings - Fork 1
Update navigation and ToolsMenu UI to match DittoSwiftTools #159
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
Conversation
|
Have we tested this on an external app? Like the demo chat app or Pos/KDS app? It looks fine here just running the standalone app, but I'd like to see it in an integration |
|
@rajramsaroop Here's what it looks like in the POS/KDS app: https://www.loom.com/share/e35ee5b1e3284b779153b913d3402d0e Ideally you'd let the TopNavBar be the header, so it looks a little off in this case, but it still provides some useful nav |
DittoToolsAndroid/src/main/java/live/ditto/tools/toolsviewer/DittoToolsViewer.kt
Show resolved
Hide resolved
thanks for doing this. i think it looks a little awkward just because of how the pos demo app implements tools. i think if it was an alert dialog/modal it would look a lot better (or something similar like a bottom sheet), but thats something a customer can easily implement |
DittoToolsAndroid/src/main/java/live/ditto/tools/toolsviewer/viewmodel/ToolsViewerViewModel.kt
Outdated
Show resolved
Hide resolved
DittoToolsAndroid/src/main/java/live/ditto/tools/toolsviewer/DittoToolsViewer.kt
Show resolved
Hide resolved
DittoToolsAndroid/src/main/java/live/ditto/tools/toolsviewer/DittoToolsViewer.kt
Outdated
Show resolved
Hide resolved
DittoToolsAndroid/src/main/java/live/ditto/tools/toolsviewer/theme/Color.kt
Show resolved
Hide resolved
8c6d43d to
7fff76c
Compare
| implementation(libs.androidx.lifecycle.runtime.compose) | ||
| implementation(platform(libs.androidx.compose.composeBom)) | ||
| implementation(libs.androidx.compose.runtime.runtimeLivedata) | ||
| implementation(libs.androidx.compose.material.icons.extended) |
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.
Can you create a linear ticket to bundle the material icons with tools, as opposed to including the library?
Out of scope for this PR.
But including the material icons as a dependency increases apk size, since its including all icons (and variants). We should be mindful to not increase a customer's apk size if not needed.
I am not sure if gradle would be smart enough to not include these resources if not needed at compile time, that might be something, as part of the ticket you create, to see if the apk size is significantly reduced by removing this library/dependency and just including the icons we want
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.
Yep, and I can test it out to see how much of a difference it makes
rajramsaroop
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.
One comment regarding creating a Linear ticket, but doesn't need to be a blocker for approving, so I'm hitting approve 👍🏾
Before:

After:
