Fix #4781: Fixed zoom in Problem with code editor #4783

bansalnitish · 2018-03-06T20:23:00Z

Explanation

Fixes #4781:
Zoom in problem is fixed.
Screenshots are attached below:

Zoom in: 150%

Zoom in: 175%

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
The PR explanation includes the words "Fixes #bugnum: ...".
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
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.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

seanlip · 2018-03-06T20:54:45Z

Btw I'm not sure that hardcoding magic numbers is the right approach. It's just going to fail on other zooms.

The idea of the code lines not extending beyond a certain width seems independent of the numerical value for that width, and I think the code should reflect that. There might be some way in codemirror to say "don't allow stuff to overflow".

bansalnitish · 2018-03-06T21:01:54Z

Hi @seanlip, I think this will work. These numbers are not randomly taken. The problem was that all the area was in one div except the logo. The div was following the rule "don't allow overflow" but the problem was the logo was outside the div by -87 px so the rule was not applicable to this logo. I just made the div larger by 87px to fit that logo in the div.
Now it's working fine and I think it will work fine for all zooms too.
Thanks

seanlip · 2018-03-06T21:04:54Z

I'm saying that typing in codemirror should not cause all the surrounding parts to change size as a matter of principle (just like me typing a long sentence into this github textarea doesn't cause it to change size). The magic numbers here are hard to maintain and no one is going to know about the significance of 87px and how it relates to the other numbers several months down the road. So I think this needs to be solved at a different level.

bansalnitish · 2018-03-06T21:16:31Z

Typing in code mirror does not increase size now since that was fixed in one of my PR.
The problem is that zooming in leads to the logo of oppia out of the screen. 87px was originally used in the code to fix the position of the logo. I have used the same and just extended the div by the amount equal to the length used to keep logo out of div.
Please correct me if I am wrong!
Thanks

seanlip · 2018-03-06T21:20:06Z

Oh, I see, thanks for explaining -- I think #4781 was described a bit vaguely.

If that's the case, a solution like this might work, but can you put comments in the CSS so that it's clear what the aim is and what the 87px refers to? Also for numbers that are derived from 87 we should explain how they are computed.

Passing the review back to @WickedBrat and @AllanYangZhou. Thanks!

bansalnitish · 2018-03-06T21:21:05Z

Sure @seanlip! Thanks 😄

codecov-io · 2018-03-06T21:54:25Z

Codecov Report

Merging #4783 into develop will increase coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4783      +/-   ##
===========================================
+ Coverage    44.73%   44.74%   +<.01%     
===========================================
  Files          385      385              
  Lines        23459    23463       +4     
  Branches      3791     3793       +2     
===========================================
+ Hits         10494    10498       +4     
  Misses       12965    12965

Impacted Files	Coverage Δ
core/templates/dev/head/app.js	`34.55% <0%> (ø)`	⬆️
...ev/head/components/alerts/AlertMessageDirective.js	`6.66% <0%> (ø)`	⬆️
core/templates/dev/head/services/AlertsService.js	`100% <0%> (ø)`	⬆️

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 735eed4...ec88ec4. Read the comment docs.

AllanYangZhou

Thanks @bansalnitish! Visually, the change looks good to me.

Code mostly looks good, just some comments/questions.

AllanYangZhou · 2018-03-07T18:39:45Z

core/templates/dev/head/pages/exploration_player/conversation_skin_directive.html

    width: 100%;
    z-index: 3;
  }

  /* Some mobile devices have CSS width below 360px, use a responsive min-width
     when under 360px to prevent pushing things offscreen. */
-  @media(max-width: 360px) {


Just wondering, I assume you're adding 87px here but what effect does that have?

Also, you should update the above comment so its clear and matches your new max-width value.

I think we don't need to add 87px here. 360px works fine as we are just fixing min-width of the div to 100vw when max-width is 360px for some mobile devices.
I have reverted the changes @AllanYangZhou .
Let me know if I am wrong here.
Thanks

AllanYangZhou · 2018-03-07T18:40:44Z

core/templates/dev/head/pages/exploration_player/progress_nav_directive.html

@@ -1,4 +1,4 @@
-<div layout="row" layout-align="space-between center" style="background-color: white;">
+<div layout="row" layout-align="space-between center" style="background-color: white; margin-left: 87px;">


As sean mentioned, don't forget to add some comments explaining the 87px.

bansalnitish · 2018-03-07T20:14:30Z

@AllanYangZhou I have added the comments.

AllanYangZhou · 2018-03-07T21:42:21Z

Great, LGTM. @WickedBrat do you have time to take a look?

* upstream/develop: (114 commits) Fix line too long (oppia#4798) Fix oppia#4374: Docstrings added to core/domain/stats_jobs_one_off.py (oppia#4757) Fixes oppia#4055: Removes redundant css selector from conversation_skin_directive.html (oppia#4790) Fix oppia#3971: add activity indicator to feedback updates (oppia#4785) Fix part of oppia#4374 : Update docstrings to core.domain.stats_domain. (oppia#4731) Fix oppia#2620 (oppia#4791) Removes unused angular dependecies injected into controllers (oppia#4792) Fix oppia#4367 : Prevent null-submission for MathExpressionInput interaction (Upgraded guppy 260da6--->b5055b) (oppia#4753) Upgrade GAE version to 1.9.67. (oppia#4786) Fix #4781: Fixed zoom in Problem with code editor (oppia#4783) Changed timeout for popup to 4 seconds. (oppia#4706) Fixes oppia#3982 : Added 'open' in the open feedback thread summary. (#4767) Fix oppia#4778: Changed is_community_owned to make_community_owned. (oppia#4780) Revert "Fixes #4530: Removes spaces and new lines from end of string in RTE. (#4748)" (oppia#4782) Fixed oppia#4537: Navigating between admin pages on url change (oppia#4777) Added $log dependency in missing places. (oppia#4772) Add missing dependency. (oppia#4769) Fix oppia#3478: Added functionality to sort nodes list. (oppia#4708) Fixes #4530: Removes spaces and new lines from end of string in RTE. (#4748) Fixes oppia#4410: Fixed info modal for private explorations (#4754) ...

This reverts commit 73be244.

…4905) This reverts commit 73be244.

…" (oppia#4905) This reverts commit 73be244.

This reverts commit 73be244.

fixed code editor

10fe072

bansalnitish requested a review from seanlip March 6, 2018 20:23

bansalnitish assigned seanlip Mar 6, 2018

seanlip requested review from AllanYangZhou and a user and removed request for seanlip March 6, 2018 20:53

seanlip assigned AllanYangZhou and ghost and unassigned seanlip Mar 6, 2018

AllanYangZhou reviewed Mar 7, 2018

View reviewed changes

requested changes

ec88ec4

seanlip merged commit 73be244 into oppia:develop Mar 7, 2018

bansalnitish deleted the editor-fix branch March 8, 2018 08:43

seanlip mentioned this pull request Apr 22, 2018

When navigating from one card to another, the animation is completely broken #4902

Closed

seanlip added a commit that referenced this pull request Apr 22, 2018

Revert "Fix #4781: Fixed zoom in Problem with code editor (#4783)"

29e58ab

This reverts commit 73be244.

DubeySandeep pushed a commit that referenced this pull request Apr 22, 2018

Revert "Fix #4781: Fixed zoom in Problem with code editor (#4783)" (#…

49ab4bb

…4905) This reverts commit 73be244.

seanlip added a commit that referenced this pull request Apr 22, 2018

Revert "Fix #4781: Fixed zoom in Problem with code editor (#4783)" (#…

ef399bf

…4905) This reverts commit 73be244.

nithusha21 mentioned this pull request May 12, 2018

Fix #4781: Fixed Code editor zoom in problem #4921

Merged

6 tasks

seanlip mentioned this pull request Jul 12, 2018

Fixing Fraction Input weird padding. #5267

Merged

6 tasks

ctao5660 pushed a commit to ctao5660/oppia that referenced this pull request Nov 23, 2018

Revert "Fix #4781: Fixed zoom in Problem with code editor (oppia#4783)…

1debda5

…" (oppia#4905) This reverts commit 73be244.

ctao5660 pushed a commit to ctao5660/oppia that referenced this pull request Nov 23, 2018

Revert "Fix #4781: Fixed zoom in Problem with code editor (oppia#4783)"

4d7752c

This reverts commit 73be244.

ghost deleted a comment Jan 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4781: Fixed zoom in Problem with code editor #4783

Fix #4781: Fixed zoom in Problem with code editor #4783

bansalnitish commented Mar 6, 2018 •

edited

Loading

seanlip commented Mar 6, 2018

bansalnitish commented Mar 6, 2018 •

edited

Loading

seanlip commented Mar 6, 2018

bansalnitish commented Mar 6, 2018

seanlip commented Mar 6, 2018

bansalnitish commented Mar 6, 2018

codecov-io commented Mar 6, 2018 •

edited

Loading

AllanYangZhou left a comment •

edited

Loading

AllanYangZhou Mar 7, 2018

bansalnitish Mar 7, 2018

AllanYangZhou Mar 7, 2018

bansalnitish Mar 7, 2018

bansalnitish commented Mar 7, 2018

AllanYangZhou commented Mar 7, 2018

		@@ -1,4 +1,4 @@
		<div layout="row" layout-align="space-between center" style="background-color: white;">
		<div layout="row" layout-align="space-between center" style="background-color: white; margin-left: 87px;">

Fix #4781: Fixed zoom in Problem with code editor #4783

Fix #4781: Fixed zoom in Problem with code editor #4783

Conversation

bansalnitish commented Mar 6, 2018 • edited Loading

Explanation

Checklist

seanlip commented Mar 6, 2018

bansalnitish commented Mar 6, 2018 • edited Loading

seanlip commented Mar 6, 2018

bansalnitish commented Mar 6, 2018

seanlip commented Mar 6, 2018

bansalnitish commented Mar 6, 2018

codecov-io commented Mar 6, 2018 • edited Loading

Codecov Report

AllanYangZhou left a comment • edited Loading

Choose a reason for hiding this comment

AllanYangZhou Mar 7, 2018

Choose a reason for hiding this comment

bansalnitish Mar 7, 2018

Choose a reason for hiding this comment

AllanYangZhou Mar 7, 2018

Choose a reason for hiding this comment

bansalnitish Mar 7, 2018

Choose a reason for hiding this comment

bansalnitish commented Mar 7, 2018

AllanYangZhou commented Mar 7, 2018

bansalnitish commented Mar 6, 2018 •

edited

Loading

bansalnitish commented Mar 6, 2018 •

edited

Loading

codecov-io commented Mar 6, 2018 •

edited

Loading

AllanYangZhou left a comment •

edited

Loading