Fix #5973: Fixed the changed space when the text is added just below the image #5996

mighty-phoenix · 2018-12-17T15:42:55Z

Explanation

Fixes #5973: The CkeditorRteDirective is changed to expected behavior.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
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.

mighty-phoenix · 2018-12-17T15:47:50Z

Kindly see the screenshots. @bansalnitish

codecov-io · 2018-12-17T18:11:26Z

Codecov Report

Merging #5996 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop   #5996   +/-   ##
=======================================
  Coverage     45.3%   45.3%           
=======================================
  Files          520     520           
  Lines        30667   30667           
  Branches      4597    4597           
=======================================
  Hits         13893   13893           
  Misses       16774   16774

Impacted Files	Coverage Δ
...plates/dev/head/components/CkEditorRteDirective.js	`1.23% <ø> (ø)`	⬆️
...ditor/statistics_tab/PlaythroughIssuesDirective.js	`4.16% <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 d48ab80...f5b76df. Read the comment docs.

bansalnitish

Hi @rakshitkumarcse -- Thanks for the PR.
This is not working as expected, notice the difference between spacing here.

Before Saving:

After Saving:

mighty-phoenix · 2018-12-17T18:34:18Z

@bansalnitish Thanks for pointing out. I thought that this behavior is what you were expecting. Because according to the process of reproducing the error, if you edit the content again, you will notice that there is no space, unlike before. Would you want me to work on it such that on saving as well, no new line appears even then? Further, of you publish the exploration, you will notice there is no extra line, and the behavior is just as expected.

So, the only question that stands is, do you want me to work further to see why the extra line is appearing on saving and fix it?

bansalnitish · 2018-12-17T18:35:25Z

Yes, exactly.

mighty-phoenix · 2018-12-17T18:43:24Z

@bansalnitish Sure. Working on it.😁

mighty-phoenix · 2018-12-17T19:15:37Z

@bansalnitish Kindly test again. It's working exactly as expected now.

bansalnitish

Hey @rakshitkumarcse, these changes look great, LGTM!

DubeySandeep · 2018-12-17T20:07:44Z

core/templates/dev/head/components/CkEditorRteDirective.js

@@ -158,7 +158,7 @@ oppia.directive('ckEditorRte', [
            } else {
              return '<div type="oppia-noninteractive-' + p3 + '"' +
                     'class="oppia-rte-component-container">' + match +
-                     '<div class="oppia-rte-component-overlay"></div></div>';


@rakshitkumarcse, can you please explain why do we have to remove this?

Hey @DubeySandeep, this was a wrapping around the block components. If you use this, then after clicking the save button and entering in the editing mode again there will be empty line between the paragraph just below it and the block component (say image).

@bansalnitish Thanks for answering.

bansalnitish

Hey @rakshitkumarcse, My bad!
Could you also follow the comment - https://github.com/oppia/oppia/pull/5996/files#diff-a5de01b7be4948894c7b8f8782677062R1483?

These classes manage the content of the response. See the screenshot below the space is being introduced due to this.

mighty-phoenix · 2018-12-17T21:32:30Z

@bansalnitish So should I revert the changes in oppia.css and let the changes in CkeditorRteDirective.js stay?

mighty-phoenix · 2018-12-17T21:38:04Z

@bansalnitish Changes in oppia.css reverted. I think we are ready for the merge.

bansalnitish · 2018-12-18T07:37:03Z

Why did you revert the changes at @rakshitkumarcse ? Did you test its working fine now (I'm quite sure, no!)?

I just asked to perform the same changes in the file conversation_skin_directive.html following the comment here - https://github.com/oppia/oppia/blob/develop/core/templates/dev/head/css/oppia.css#L1483

If you don't do the same changes in the mentioned file then the issue still persists (in response). You could reproduce as follows:

Create an exploration with say Item selection interaction.
Add Response for an answer by the user. In this response do the same - add an image and then enter the text below it.
Run this in the preview mode. You'll see the something similar to screenshots above.

Let me know if its still unclear. Thanks!

mighty-phoenix · 2018-12-18T08:22:57Z

@bansalnitish Uh oh. Sorry. Was confused with your earlier reply. Will make the required changes.
Thanks for clearing!

mighty-phoenix · 2018-12-18T08:28:54Z

@bansalnitish Kindly merge. Thanks!

bansalnitish · 2018-12-18T09:19:08Z

Why kept the same css here - https://github.com/oppia/oppia/blob/develop/core/templates/dev/head/css/oppia.css#L1508? Any specific reason?

(Also, please do check that it works fine for all interactions, where ever the RTE is being used and on mobile as well as desktop. Don't rush to get this in. I'll check on my part too but its good to be tested by two.)

mighty-phoenix · 2018-12-18T09:50:13Z

@bansalnitish As for the same css, I don't see the class concerned being used anywhere at all in any html file. So no idea why the same css is used there. Could it be possible that it is just incorporated in oppia.css for some new feature our fellow developers are working on?

mighty-phoenix · 2018-12-18T10:14:26Z

@bansalnitish I have tested all interactions that can contain images and the result looks pretty fine to me.
Thanks!

bansalnitish · 2018-12-18T18:01:41Z

Hey @rakshitkumarcse, This is looking really good and we are close.

Nope, this is not added for some developer working on any future feature. I'd say remove the margins involved in the class .oppia-rte-content > div > p .

The reason for this is we have a p tag in oppia.css and there we have specified the margin-bottom. Now, removing the margins here would mean it would inherit margins from this p tag, which would work fine (as its working in our current case, which just need margin-bottom).

mighty-phoenix · 2018-12-18T20:07:04Z

@bansalnitish Thanks for the clarification. It is clear to me now.
I have made the changes you requested.

bansalnitish

Thank you @rakshitkumarcse, LGTM!

mighty-phoenix · 2018-12-19T08:44:11Z

@DubeySandeep @bansalnitish kindly merge.

Fixed

72dce1b

bansalnitish reviewed Dec 17, 2018

View reviewed changes

Correction

eb239a9

bansalnitish approved these changes Dec 17, 2018

View reviewed changes

DubeySandeep requested changes Dec 17, 2018

View reviewed changes

DubeySandeep approved these changes Dec 17, 2018

View reviewed changes

bansalnitish suggested changes Dec 17, 2018

View reviewed changes

Revert changes in oppia.css

319ef35

Final changes

678527a

Final change

f5b76df

bansalnitish approved these changes Dec 18, 2018

View reviewed changes

bansalnitish merged commit 05ed54f into oppia:develop Dec 19, 2018

mighty-phoenix deleted the fixed_space_issue branch December 19, 2018 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #5973: Fixed the changed space when the text is added just below the image #5996

Fix #5973: Fixed the changed space when the text is added just below the image #5996

mighty-phoenix commented Dec 17, 2018 •

edited

Loading

mighty-phoenix commented Dec 17, 2018 •

edited

Loading

codecov-io commented Dec 17, 2018 •

edited

Loading

bansalnitish left a comment

mighty-phoenix commented Dec 17, 2018

bansalnitish commented Dec 17, 2018

mighty-phoenix commented Dec 17, 2018

mighty-phoenix commented Dec 17, 2018

bansalnitish left a comment

DubeySandeep Dec 17, 2018

bansalnitish Dec 17, 2018

mighty-phoenix Dec 17, 2018

DubeySandeep Dec 17, 2018

bansalnitish left a comment

mighty-phoenix commented Dec 17, 2018

mighty-phoenix commented Dec 17, 2018

bansalnitish commented Dec 18, 2018

mighty-phoenix commented Dec 18, 2018

mighty-phoenix commented Dec 18, 2018

bansalnitish commented Dec 18, 2018 •

edited

Loading

mighty-phoenix commented Dec 18, 2018

mighty-phoenix commented Dec 18, 2018

bansalnitish commented Dec 18, 2018

mighty-phoenix commented Dec 18, 2018

bansalnitish left a comment

mighty-phoenix commented Dec 19, 2018 •

edited

Loading

Fix #5973: Fixed the changed space when the text is added just below the image #5996

Fix #5973: Fixed the changed space when the text is added just below the image #5996

Conversation

mighty-phoenix commented Dec 17, 2018 • edited Loading

Explanation

Checklist

mighty-phoenix commented Dec 17, 2018 • edited Loading

codecov-io commented Dec 17, 2018 • edited Loading

Codecov Report

bansalnitish left a comment

Choose a reason for hiding this comment

mighty-phoenix commented Dec 17, 2018

bansalnitish commented Dec 17, 2018

mighty-phoenix commented Dec 17, 2018

mighty-phoenix commented Dec 17, 2018

bansalnitish left a comment

Choose a reason for hiding this comment

DubeySandeep Dec 17, 2018

Choose a reason for hiding this comment

bansalnitish Dec 17, 2018

Choose a reason for hiding this comment

mighty-phoenix Dec 17, 2018

Choose a reason for hiding this comment

DubeySandeep Dec 17, 2018

Choose a reason for hiding this comment

bansalnitish left a comment

Choose a reason for hiding this comment

mighty-phoenix commented Dec 17, 2018

mighty-phoenix commented Dec 17, 2018

bansalnitish commented Dec 18, 2018

mighty-phoenix commented Dec 18, 2018

mighty-phoenix commented Dec 18, 2018

bansalnitish commented Dec 18, 2018 • edited Loading

mighty-phoenix commented Dec 18, 2018

mighty-phoenix commented Dec 18, 2018

bansalnitish commented Dec 18, 2018

mighty-phoenix commented Dec 18, 2018

bansalnitish left a comment

Choose a reason for hiding this comment

mighty-phoenix commented Dec 19, 2018 • edited Loading

mighty-phoenix commented Dec 17, 2018 •

edited

Loading

mighty-phoenix commented Dec 17, 2018 •

edited

Loading

codecov-io commented Dec 17, 2018 •

edited

Loading

bansalnitish commented Dec 18, 2018 •

edited

Loading

mighty-phoenix commented Dec 19, 2018 •

edited

Loading