Fix #5949: Make RTE spacing uniform #5953

ankita240796 · 2018-12-06T17:27:09Z

Explanation

Fixes #5949: Adding bottom margin for paragraphs in editor mode fixes the issue of inconsistent spacing.

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.

ankita240796 · 2018-12-06T17:28:16Z

Hi @seanlip, @bansalnitish PTAL!

codecov-io · 2018-12-06T18:08:26Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@e93451b). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #5953   +/-   ##
==========================================
  Coverage           ?   45.47%           
==========================================
  Files              ?      517           
  Lines              ?    30378           
  Branches           ?     4571           
==========================================
  Hits               ?    13814           
  Misses             ?    16564           
  Partials           ?        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 e93451b...8762210. Read the comment docs.

bansalnitish

Hi @ankita240796,
You cleared of the paragraph spaces that were indeed needed in the preview mode. The expected thing to do here was to add the spaces at the start of next paragraph in the editor mode (though in the preview mode it works fine).

You cleared off the spaces that should be there at the start of paragraph in the preview mode. Let me know if you need some more clarification on this one.

ankita240796 · 2018-12-06T20:04:08Z

Hi @bansalnitish, I have made the changes as suggested by you. PTAL!

bansalnitish

Looks good!
A minor nit and also one thing to note here is that we don't need bottom margin for the last paragraph. You could try something like:

.oppia-rte > p:not(:last-child) {
  line-height: 1.5;
  margin-bottom: 18px;
}

bansalnitish · 2018-12-06T20:07:34Z

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

  */
  .rte-viewer > p {
    line-height: 28px;
+    margin-bottom: 18px;
+    margin-top: 18px


Missing Semicolon!

Done, PTAL!

ankita240796 · 2018-12-07T06:07:49Z

Hi @bansalnitish, I have made the changes, PTAL!

bansalnitish · 2018-12-07T07:26:41Z

Hi @ankita240796,

This works fine in the editor mode but just try creating an item selection exploration and click the preview button. All those paragraph spaces are gone! This needs to be fixed.

Screenshot:
This is what I saved.

This is what I got.

ankita240796 · 2018-12-07T08:19:34Z

Hi @bansalnitish,

I have made the paragraph margin consistent for all p tags. I also tested it for various interactions and hints as well. It works fine now, PTAL!

Thanks!

bansalnitish

This looks like a good fix @ankita240796. LGTM!

nithusha21

In the editor view, when not in editing mode:

In the editor view, while editing:

In the preview tab:

I think the spacing is different while editing, while the spacing matches on the editor and preview tabs.
The spacing below the image also differs.
In the original issue report, the comparison was between the "while editing" stage and the "not editing" state of the editor view.

To me it still looks like the spacing is different. Is this similar to what you see @bansalnitish? If so I think the issue is still not fixed...

nithusha21 · 2018-12-08T09:28:24Z

core/templates/dev/head/css/oppia.css

  text-align: left;
  word-spacing: 0;
  word-break: break-word;
 }

+p:last-child {


Hi @bansalnitish, @ankita240796 I think this will affect all paragraph tags on Oppia right? Is this intended? Just clarifying...

Yes @nithusha21 , it affects the behaviour of all paragraph tags on Oppia but it provides a consistent behaviour as well. I checked for various pages and interactions and the change does not create any issues.

bansalnitish

Just to avoid merge!

ankita240796 · 2018-12-08T15:59:11Z

Hi @bansalnitish @nithusha21,

I tried some other stuff as follows:

We can fix p tags in rte editor using the following css:

.oppia-rte > p {
  line-height: 1.5;
  margin-bottom: 18px;
  margin-top: 18px;
}

.oppia-rte > p:last-child {
  margin-bottom: 0px;
}

.oppia-rte > p:first-child {
  margin-top: 0px;
}

This introduces issues with multiple choice and item selection interaction where margin is seen in editor but not in preview. To fix that we can use following css:

.protractor-test-multiple-choice-option > p {
  line-height: 1.5;
  margin-bottom: 18px;
  margin-top: 18px;
}

.protractor-test-multiple-choice-option > p:last-child {
  margin-bottom: 0px;
}

.protractor-test-multiple-choice-option > p:first-child {
  margin-top: 0px;
}

Further this leaves p tags without margin in add response modal which are not nested in some specific class. Here is a screenshot for the same:

We can either introduce a class or use the angular-html-bind tag to enforce the rule but the second option is less preferred since it can have a vast scope for other p tags (If we do not want to modify p tags everywhere).

Can you please provide your inputs here about how can I further approach this issue.

Thanks!

nithusha21 · 2018-12-11T10:00:19Z

Hmmm, I am not sure. Deferring to @bansalnitish

bansalnitish · 2018-12-12T08:34:55Z

@nithusha21 to me the spacing looks good, I have some screenshots for the same. I also checked the spacing between paragraphs via the inspect element and it's 18 px everywhere.

In the editor mode while editing:

In the editor mode:

In the preview mode:

In the editor mode while editing:

In the editor mode:

In the preview tab:

nithusha21 · 2018-12-14T07:42:15Z

Ah I see..... The paragraph spacing looks good to me. I think I was confused 😅. I was looking at the line spacing (Within a paragraph). And that still looks off to me. Can we file an issue for that? (If you can confirm the behavior, and also check if there already exists an issue for the same).

nithusha21 · 2018-12-14T07:42:38Z

Also, thanks for filing the issue for image spacing I pointed out! 😄

bansalnitish · 2018-12-14T07:53:01Z

Yes, you're right @nithusha21 the line spacing in the editor mode while editing and after saving look different to me as well. I'll check once again and file an issue (if it exists).

bansalnitish

Thanks @ankita240796 for the PR, LGTM!

fix extra space in rte

209a2ea

nithusha21 requested a review from bansalnitish December 6, 2018 17:46

nithusha21 assigned bansalnitish Dec 6, 2018

Remove margin for p in other rte views as well

e8629c4

bansalnitish suggested changes Dec 6, 2018

View reviewed changes

Make paragraph spacing uniform

4ce6818

ankita240796 changed the title ~~Fix #5949: Remove extra space in rte~~ Fix #5949: Make RTE spacing uniform Dec 6, 2018

bansalnitish suggested changes Dec 6, 2018

View reviewed changes

Addressed review comments

d587d90

Add margin for all paragraphs to remove inconsistency

8762210

bansalnitish approved these changes Dec 7, 2018

View reviewed changes

nithusha21 reviewed Dec 8, 2018

View reviewed changes

bansalnitish suggested changes Dec 8, 2018

View reviewed changes

nithusha21 added PR: don't merge - NEEDS CLA and removed PR: don't merge - NEEDS CLA labels Dec 9, 2018

bansalnitish approved these changes Dec 14, 2018

View reviewed changes

bansalnitish merged commit b4fd79b into oppia:develop Dec 14, 2018

ankita240796 deleted the fix-rte branch December 14, 2018 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #5949: Make RTE spacing uniform #5953

Fix #5949: Make RTE spacing uniform #5953

ankita240796 commented Dec 6, 2018 •

edited

Loading

ankita240796 commented Dec 6, 2018

codecov-io commented Dec 6, 2018 •

edited

Loading

bansalnitish left a comment

ankita240796 commented Dec 6, 2018

bansalnitish left a comment

bansalnitish Dec 6, 2018

ankita240796 Dec 7, 2018

ankita240796 commented Dec 7, 2018

bansalnitish commented Dec 7, 2018 •

edited

Loading

ankita240796 commented Dec 7, 2018

bansalnitish left a comment

nithusha21 left a comment

nithusha21 Dec 8, 2018

ankita240796 Dec 8, 2018

bansalnitish left a comment

ankita240796 commented Dec 8, 2018 •

edited

Loading

nithusha21 commented Dec 11, 2018

bansalnitish commented Dec 12, 2018

nithusha21 commented Dec 14, 2018

nithusha21 commented Dec 14, 2018

bansalnitish commented Dec 14, 2018

bansalnitish left a comment

Fix #5949: Make RTE spacing uniform #5953

Fix #5949: Make RTE spacing uniform #5953

Conversation

ankita240796 commented Dec 6, 2018 • edited Loading

Explanation

Checklist

ankita240796 commented Dec 6, 2018

codecov-io commented Dec 6, 2018 • edited Loading

Codecov Report

bansalnitish left a comment

Choose a reason for hiding this comment

ankita240796 commented Dec 6, 2018

bansalnitish left a comment

Choose a reason for hiding this comment

bansalnitish Dec 6, 2018

Choose a reason for hiding this comment

ankita240796 Dec 7, 2018

Choose a reason for hiding this comment

ankita240796 commented Dec 7, 2018

bansalnitish commented Dec 7, 2018 • edited Loading

ankita240796 commented Dec 7, 2018

bansalnitish left a comment

Choose a reason for hiding this comment

nithusha21 left a comment

Choose a reason for hiding this comment

nithusha21 Dec 8, 2018

Choose a reason for hiding this comment

ankita240796 Dec 8, 2018

Choose a reason for hiding this comment

bansalnitish left a comment

Choose a reason for hiding this comment

ankita240796 commented Dec 8, 2018 • edited Loading

nithusha21 commented Dec 11, 2018

bansalnitish commented Dec 12, 2018

nithusha21 commented Dec 14, 2018

nithusha21 commented Dec 14, 2018

bansalnitish commented Dec 14, 2018

bansalnitish left a comment

Choose a reason for hiding this comment

ankita240796 commented Dec 6, 2018 •

edited

Loading

codecov-io commented Dec 6, 2018 •

edited

Loading

bansalnitish commented Dec 7, 2018 •

edited

Loading

ankita240796 commented Dec 8, 2018 •

edited

Loading