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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for style w/o location and single style #990
Conversation
This change will apply a style even if the receiver doesn't have a fixed location or if only a day or night style is provided.
d335e3e
to
f99ffcb
Compare
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 nit, looks good otherwise.
} | ||
|
||
// Single style usage | ||
guard styles.count > 1 else { |
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.
Is this block really needed? You could easily do the same thing below via:
for style in styles {
if style.styleType == styleTypeForTimeOfDay || styles.count == 1 {
style.apply()
currentStyleType = style.styleType
delegate?.styleManager?(self, didApply: style)
}
}
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.
Yes less code is always nice, but IMO explicit guards make things a little more clear and they are easier to debug.
MapboxNavigation/StyleManager.swift
Outdated
// We can't calculate sunset or sunrise w/o a location so just apply all styles | ||
for style in styles { | ||
style.apply() | ||
delegate?.styleManager?(self, didApply: style) |
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.
In the case where we cannot find a style, we're going to rapid fire go through all supplied styles and apply them? What if we just applied the first?
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.
I assume styles.count would mostly be 1 but I could imagine e.g. [TopBarDayStyle(), BottomBarDayStyle(), FloatingButtonsDayStyle()]
if one class would become unwieldy so applying all styles seems appropriate. Maybe applying all styles that have the same styleType
would be better? 馃
Fixes #989
This change will apply a style even if the receiver doesn't have a fixed
location or if only a day or night style is provided.
@bsudekum @ericrwolfe 馃憖