✨ Add generic settings panel #17
Conversation
julianharty
left a comment
julianharty
left a comment
There was a problem hiding this comment.
Overall the code looks OK to merge. I've provided various comments mainly as I'm thinking ahead for how this app might be used by manufacturers and by end-users. Once you've read them you can action any that are worth addressing before merging; some of the others might be worth writing up as github issues.
|
|
||
| Box( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| ) { |
There was a problem hiding this comment.
These previous 3 lines look unnecessarily spread out; is there a reason why they're not on the same line?
| Text( | ||
| text = value, | ||
| color = Color.White, | ||
| color = if (isSelected) Color(0xFFFFCC00) else Color.White, |
There was a problem hiding this comment.
Presumably hard-coded colours are good enough for the moment? Would either of these colours ever change e.g. if the user has their device set to high-contrast mode?
| y = visibleRegion!!.top.dp | ||
| ) | ||
| .border(2.dp, Color.Yellow) | ||
| .border(2.dp, Color(0xAAFFCC00)) |
There was a problem hiding this comment.
Again I'm curious why this is a hard-coded colour...?
| value = null | ||
| clearValueRunnable = null | ||
| } | ||
| handler.postDelayed(clearValueRunnable!!, 5000) |
There was a problem hiding this comment.
As part of making the code customisable for various manufacturers to use, how useful would it be to make these sorts of values easy to specify/modify (rather than someone searching through all the code and then editing values as they find them)?
| render = { value, isSelected -> | ||
| Text( | ||
| text = value, | ||
| color = if (isSelected) Color(0xFFFFCC00) else Color.White, |
There was a problem hiding this comment.
As per my earlier comment on hard-coding colours.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Somewhat tongue-in-cheek, 9 levels of nesting seems excessive. Of course, the code needs each of these closing braces, I'm more curious why we end up so deeply nested to provide the functionality.
| showSettingsTray = true | ||
| }, | ||
| modifier = Modifier | ||
| .size(34.dp) |
There was a problem hiding this comment.
This and similar values seem ripe for someone wanting to twiddle with them to make the UI work on various screen resolutions and devices, yet they're hard-coded.
ISNIT0
force-pushed
the
ISNIT0/settings-panel
branch
from
80fb0da to
95acdc7
Compare
A first pass of general settings panel