Skip to content

InstructionView: not return when showSubLayout has been called. #3137

Merged
JunDai merged 1 commit into
masterfrom
jd-quick-fix
Jun 9, 2020
Merged

InstructionView: not return when showSubLayout has been called. #3137
JunDai merged 1 commit into
masterfrom
jd-quick-fix

Conversation

@JunDai
Copy link
Copy Markdown
Contributor

@JunDai JunDai commented Jun 8, 2020

Description

Ref #3133

This PR fixes an issue mentioned in #3133 which is a different issue.

  • I have added any issue links
  • I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
  • I have added the appropriate milestone and project boards

Goal

Improve UI SDK quality.

Implementation

In the original implementation #1440, it added the return statement. @Guardiola31337 , if you know anything about why it's needed please let me know.

But from my testing, in the route of your comment #3133 (comment)

In the route there are two consecutive bannerInstructions, both have sub node.
The first sub node is for turnLanes. The second sub node is to show the subText.
The existing implementation will return after it shows the subText. So the turnLanes doesn't have chance to hide itself.

Screenshots or Gifs

ezgif com-video-to-gif (3)

Testing

See above gif.

  • I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
  • I have tested via a test drive, or a simulation/mock location app
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have updated the CHANGELOG including this PR

@JunDai JunDai added bug Defect to be fixed. ✓ ready for review labels Jun 8, 2020
@JunDai JunDai requested a review from Guardiola31337 June 8, 2020 21:53
@JunDai JunDai self-assigned this Jun 8, 2020
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #3137 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3137   +/-   ##
=========================================
  Coverage     38.67%   38.67%           
  Complexity     2248     2248           
=========================================
  Files           542      542           
  Lines         19735    19735           
  Branches       1866     1866           
=========================================
  Hits           7633     7633           
  Misses        11221    11221           
  Partials        881      881           

…urnLanes will be removed after it.

In a case that both the two consecutive bannerInstructions have "sub" node. The first "sub" node is
to show turnLanes and the second "sub" node is to show the subText. The existing code will show
turnLanes for first "sub" node. Then when it shows the second "sub" node as subText, it "return"s
immediately. The first "sub" node doesn't have chance to hide itself.
Copy link
Copy Markdown
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

instructionLoader.loadInstruction();
}
showSubLayout();
hideTurnLanes();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!

@JunDai JunDai merged commit 824be98 into master Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Defect to be fixed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants