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.

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

Message-view: Fix excessive spacing for lists followed by paragraphs. #4495

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rajprakash00
Copy link
Contributor

@rajprakash00 rajprakash00 commented Feb 23, 2021

Previously there was an excessive space for ordered lists coming after
a paragraph unlike plain bulleted lists.This was already fixed in webapp
message-list view in #17285

We fixed this in mobile app by following same spacing as of bulleted
lists coming after paragraph.

Screenshots:
Before:
zulipBefore3

After:
zulipAfter3

Fixes: #4492
Signed-off-by: rajprakash00 [email protected]

@rajprakash00
Copy link
Contributor Author

@gnprice Please take a look

@gnprice
Copy link
Member

gnprice commented Apr 29, 2021

Thanks @rajprakash00 !

Would you please post both a "before" screenshot as well as an "after" screenshot of the same test data? That would make this a lot easier to review.

I like the idea of treating ul and ol the same. When I look at the webapp's rendered_markdown.css, that's what we seem to do there.

I notice in this code that there's another existing rule that also refers to ul, and it doesn't get updated in this commit:

 p {
   margin: 0;
 }
 .message p + p {
   margin-top: 16px;
 }
-.message ul {
+.message ul, ol {
   padding-left: 20px;
   margin: 4px 0 0 0;
 }
 .message ul + p {
   margin-top: 16px;
 }

Is there a reason to leave that one unchanged? Other than making ul and ol still not quite treated the same, I wonder what the effect is for ol of having just one of these rules apply and not the other.

Previously there was an excessive space for ordered lists coming after
a paragraph unlike plain bulleted lists.This was already fixed in webapp
message-list view in #17285.

We fixed this in mobile app by following same spacing as of bulleted
lists coming after paragraph.

Fixes: zulip#4492
Signed-off-by: rajprakash00 <[email protected]>
@rajprakash00
Copy link
Contributor Author

Thanks @gnprice for the review.

I have added the before screenshot .

Hmm, I tested this just now and ul and ol styles becomes different when a paragraph comes after that, and you are right about treating ol and ul same , it should be same as of ul + p style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Excessive space around numbered list
2 participants