Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #4350, #4217, #4361: Enable progress-nav on desktop (and some other interaction fixes) #4365

Merged
merged 12 commits into from
Jan 6, 2018
Merged

Fix #4350, #4217, #4361: Enable progress-nav on desktop (and some other interaction fixes) #4365

merged 12 commits into from
Jan 6, 2018

Conversation

seanlip
Copy link
Member

@seanlip seanlip commented Jan 5, 2018 •
edited
Loading

Supersedes and builds on #4335 (thanks @AllanYangZhou!)

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
  • The linter/Karma presubmit checks have passed.
  • The PR is made from a branch that's not called "develop".
  • The PR follows the style guide.
  • The PR is assigned to an appropriate reviewer.

@seanlip seanlip requested a review from AllanYangZhou January 5, 2018 07:10
@seanlip
Fully fix everything on desktop, mobile and embed views. Enforce that…
643b15e 
… demo exploration covers all interactions. Remove unnecessary code.
@codecov-io
Copy link

codecov-io commented Jan 5, 2018 •
edited
Loading

Codecov Report

Merging #4365 into develop will increase coverage by 0.09%.
The diff coverage is 1.78%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4365      +/-   ##
===========================================
+ Coverage    44.57%   44.66%   +0.09%     
===========================================
  Files          376      375       -1     
  Lines        23108    23038      -70     
  Branches      3684     3663      -21     
===========================================
- Hits         10300    10291       -9     
+ Misses       12808    12747      -61
Impacted Files Coverage Δ
...sions/interactions/CodeRepl/directives/CodeRepl.js 34.05% <ø> (+0.25%) ⬆️
...ions/MusicNotesInput/directives/MusicNotesInput.js 53.96% <ø> (-0.49%) ⬇️
...temSelectionInput/directives/ItemSelectionInput.js 35.59% <ø> (+2.25%) ⬆️
...hExpressionInput/directives/MathExpressionInput.js 8.97% <ø> (+0.43%) ⬆️
...teractions/NumericInput/directives/NumericInput.js 39.21% <ø> (+2.85%) ⬆️
...ractions/FractionInput/directives/FractionInput.js 35.8% <ø> (+1.68%) ⬆️
...ons/interactions/TextInput/directives/TextInput.js 56.52% <ø> (+3.09%) ⬆️
...sions/interactions/Continue/directives/Continue.js 20% <ø> (+2.75%) ⬆️
...s/interactions/LogicProof/directives/LogicProof.js 8.73% <ø> (+0.26%) ⬆️
...ges/exploration_editor/editor_tab/StateSolution.js 1.25% <0%> (-0.14%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9299ff5...683a026. Read the comment docs.

This was referenced Jan 5, 2018
Closed
Closed
@seanlip seanlip changed the title Fix #4350: Enable progress-nav on desktop Fix #4350, #4217, #4361: Enable progress-nav on desktop (and some other interaction fixes) Jan 5, 2018
@seanlip
Copy link
Member Author

seanlip commented Jan 5, 2018 •
edited
Loading

@AllanYangZhou this is now ready for review. PTAL when you get a chance!

I am aware of the following issues, but will file separate issues for them. They are not urgent fixes.

  • Can't move or add edge for GraphInput in mobile (I think this is probably an old bug)
  • Can't submit solutions for MathExpressionInput and MusicNotesInput in editor (the answer validity checks are tricky here because the variables aren't bound well to Angular)
  • SetInput needs disabling (I ran into the bug you encountered and disabled the disabling)

I think everything else works correctly (on desktop, mobile, embed, and submitting solutions in the editor view) but it would be great if you could help verify. Thanks!

@anmolshkl anmolshkl mentioned this pull request Jan 5, 2018
Closed
@seanlip seanlip mentioned this pull request Jan 5, 2018
Closed
@Nix07 Nix07 mentioned this pull request Jan 6, 2018
Closed
5 tasks
AllanYangZhou
AllanYangZhou approved these changes Jan 6, 2018
View reviewed changes
Copy link
Contributor

@AllanYangZhou AllanYangZhou left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested it locally also. Yeah I think the GraphInput bug predates the progress-nav work.

Thanks Sean!

core/templates/dev/head/pages/exploration_player/ProgressNavDirective.js Outdated
interactionHasNavSubmitButton = INTERACTION_SPECS[
interaction.id].show_nav_submit_button;
interactionHasNavSubmitButton = (
Boolean(interaction.id) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, this is because the id might not exist in solution editor?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh no. This is for the editor view. If you create a new exploration and then preview it immediately, there's no interaction specified yet. This was causing a failure in one of the e2e tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah OK I see, thanks!

@seanlip
Copy link
Member Author

seanlip commented Jan 6, 2018

Thanks for the review @AllanYangZhou! I'll sort out the merge conflicts and then get this in.

@aks681, this should unblock your upcoming PR!

@AllanYangZhou AllanYangZhou mentioned this pull request Jan 6, 2018
Closed
@seanlip seanlip merged commit d6aabd5 into develop Jan 6, 2018
@seanlip seanlip deleted the navbar-for-desktop branch January 6, 2018 10:37
@nithusha21 nithusha21 mentioned this pull request Feb 15, 2018
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AllanYangZhou AllanYangZhou AllanYangZhou approved these changes

Assignees

@AllanYangZhou AllanYangZhou

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seanlip @codecov-io @AllanYangZhou