- Notifications
You must be signed in to change notification settings - Fork320
Added exit signs to the instruction banner and refactored instruction loader#1195
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
87afe0b toad4c8d4Compareakitchen commentedAug 20, 2018
Related to#972 |
danesfeder commentedAug 20, 2018
@devotaaabel are the tests updated here? I see some were removed from |
ad4c8d4 tof73829cCompare75b6294 to0885230Comparecodecov-io commentedDec 20, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #1195 +/- ##============================================- Coverage 29.32% 28.86% -0.47%+ Complexity 820 815 -5============================================ Files 203 212 +9 Lines 7924 7993 +69 Branches 619 622 +3 ============================================- Hits 2324 2307 -17- Misses 5379 5468 +89+ Partials 221 218 -3 |
de73fa7 tod4a7864Comparedevotaaabel commentedJan 2, 2019
@Guardiola31337@danesfeder This is ready for review. I still want to do some more thorough testing. I cleaned up the unit tests and made them a little more useful, but I need help understanding the code cov report, I'm not sure how to see the changes that I made that it's complaining about. I could add tests to the Verifiers and Nodes, but that doesn't seem very useful, but I can do it if we need it for code coverage. |
This comment has been minimized.
This comment has been minimized.
a571be1 to8ae2926Compare4d88d8c to21ae19bCompare
danesfeder 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.
@devotaaabel thanks for addressing the feedback, I have no further comments. We should mark this asSEMVER due to the renaming ofImageCoordinator >ImageCreator. Also can we cut a ticket to track the styling of the exits if that's considered out-of-scope for this PR?
devotaaabel commentedJan 28, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Created an issue here#1705 will open another PR after this PR lands. Thanks! |
Guardiola31337 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.
This is looking great@devotaaabel
While testing this, I've noticed some weird issues that we need to 👀 before landing this 👇
This seems not abbreviated properly
Left exit icon not drawn
I also left some minor comments / questions.
| AbbreviationCoordinator() { | ||
| this(newTextViewUtils()); | ||
| AbbreviationCreator(AbbreviationVerifierabbreviationVerifier) { |
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.
As discussed internally, should we remove this constructor? IMO it's not adding any value.
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.
@Guardiola31337 What discussion? Are you talking about the discussion about Dan's PR? If you are, you said that you didn't feel strongly about that either way. I think this is adding the value of not making theInstructionLoader instantiate the hashmap and text utils while still making this testable. It's basically acting as a dependency injection utility method
| @Override | ||
| BannerComponentNodesetupNode(BannerComponentscomponents,intindex,intstartIndex,String | ||
| modifier) { | ||
| if (components.type().equals("exit")) { |
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.
Have we thought about extracting these magic numbers i.e."exit","exit-number" and"left" into constants?
...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/NodeVerifier.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Going to hold on the approval until we do some more field testing.
Guardiola31337 commentedJan 29, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I kept testing this locally and I found the following 👀 Pixel XL Android 9 Exit sign loaded intermittently
Abbreviation issue and hidden secondary banner
Primary text empty
Shield not loaded
Saw the following log trace when ☝️ happened |
f41f0fa tode9a31aCompare20a59cb to92bc66aComparedevotaaabel commentedJan 30, 2019
@danesfeder So I like this idea, but I'm thinking, let's not do that until there's a need for it, either on our side or a customer request, unless there is one that I'm not aware of. Just to keep it simple for now until there's a need. Let me know what you think. |
Guardiola31337 commentedJan 30, 2019
After rolling back abbreviations fixes7be09d2 I wasn't able to reproduce the issues found in#1195 (comment) so it seems that was the reason. Per internal discussion we're going to keep discussing on a solution for landscape issues and will follow up in a different PR. I was able to reproduce theLeft exit icon not drawn#1195 (review) though. Apparently, there's some issue with Android 6.0 and rotating the vector to the left. We've tested and we're going to generate a new left asset to workaround this. |
devotaaabel commentedJan 30, 2019
@Guardiola31337 The left exit android 6.0 fix is pushed |
devotaaabel commentedJan 30, 2019
@Guardiola31337@danesfeder This is ready for re-review, I've addressed all relevant issues |
danesfeder 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.
@devotaaabel great work here and thanks for addressing the feedback 👍
ef9943b to5f477aeCompare
Uh oh!
There was an error while loading.Please reload this page.
Left to do:
Moved style attributes into separate branch. Will make a PR when this one lands. Issue is here:#1705
I wanted to add more tests but it is difficult with the nature of the image spans, and I'm assuming that's why there weren't any tests for the highway signs. If anyone has any advice on how to unit test, it would be helpful. Otherwise I'm going to leave it out.
Closes#972