UI improvements #948
UI improvements #948
Conversation
cla-bot
bot
added
the
cla-signed ✔️
| @@ -60,7 +61,7 @@ internal class TripDrawer(context: Context) : MapDrawer(context) { | |||
|
|
|||
| // get colors | |||
| val backgroundColor = getBackgroundColor(leg) | |||
| val foregroundColor = getForegroundColor(leg) | |||
| val foregroundColor = TransportrUtils.getTextColorBasedOnBackground(backgroundColor) | |||
There was a problem hiding this comment.
Is it a good idea that this completely ignores the foreground color? I would expect something more like "If the foreground is too similar to the background, invert it".
There was a problem hiding this comment.
I don't get what you mean, this line sets the foregroundcolor (see corresponding method, to either white or black depending on what is better^^)...
There was a problem hiding this comment.
Sorry, I meant that it does not take into consideration leg.line.style.foregroundColor.
There was a problem hiding this comment.
The line style in the context of the TripDrawer, is being considered in the backgroundColor variable through getBackgroundColor()
app/src/main/java/de/grobox/transportr/trips/detail/TripDrawer.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/grobox/transportr/trips/detail/TripUtils.kt
Outdated
Show resolved
Hide resolved
| // get colors | ||
| val foregroundColor = line.style?.foregroundColor | ||
| val backgroundColor = line.style?.backgroundColor | ||
| val foregroundColor = getTextColorBasedOnBackground(backgroundColor) |
There was a problem hiding this comment.
Is it a good idea that this completely ignores the foreground color? I would expect something more like "If the foreground is too similar to the background, invert it".
| import de.schildbach.pte.dto.Line | ||
|
|
||
| class LineView(context: Context, attr: AttributeSet?) : AppCompatTextView(context, attr) { | ||
|
|
||
| fun setLine(line: Line) { | ||
| fun setLine(line: Line, backgroundColor: Int) { |
There was a problem hiding this comment.
imo a setter ("setLine") receiving having more than one parameter is very unintuitive and makes for the caller code harder to read. I would expect the function to be called setLineOverBackground (or something like that) and/or to explicitly give the parameter name at the caller's site (setLine(leg.line, backgroundColor=foo))
There was a problem hiding this comment.
Yes I can rename it if its cleaner that way :)
There was a problem hiding this comment.
Well I thought about it, and for it makes sense like it is right now.
The parameter called "backgroundColor" here, actually just represents the color of the line. It doesn't represent some "background" over which the "line" would be drawn. Does it make more sense with this explanation? I could add some comment in the code to explain this a little bit more :)
| val colorsToCheck = listOfNotNull( | ||
| line.style?.backgroundColor, | ||
| line.style?.backgroundColor2, | ||
| line.style?.foregroundColor, | ||
| line.style?.borderColor | ||
| ) |
There was a problem hiding this comment.
What's the thought process behind this? This doesn't seem to align with the behavior before in any way. For example, why is the background color a "higher priority" than the foreground color?
There was a problem hiding this comment.
These are line colors, and I think I followed the current behavior well, see here:
Fixes:
Improves:
Screenshots: