Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix spacing problem of paragraphs around lists #10305

Closed
wants to merge 17 commits into from
Closed

Fix spacing problem of paragraphs around lists #10305

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
39ca0e3
Add margin-top to paragraph below ul or ol
luixxiul Mar 7, 2023
b96df96
Add margin to the list on compact modern layout
luixxiul Mar 7, 2023
613040f
Sort style blocks
luixxiul Mar 7, 2023
2525350
Ensure the same value is applied to both top and bottom margin
luixxiul Mar 7, 2023
d9ebe6e
Edit comments
luixxiul Mar 7, 2023
0578f2c
Add a test - Send lists with paragraphs as a message
luixxiul Mar 7, 2023
2e8156a
Check block margin on modern (compact) layout
luixxiul Mar 7, 2023
b34692f
Check block margin on IRC layout
luixxiul Mar 7, 2023
c4d13ef
Check block margin on bubble layout
luixxiul Mar 7, 2023
951a210
Use functions
luixxiul Mar 7, 2023
09f5cfd
Add percySnapshotElement
luixxiul Mar 7, 2023
dbe57d6
Edit a comment
luixxiul Mar 7, 2023
96be6f7
Merge branch 'develop' into fix-margin-list
luixxiul Mar 7, 2023
934e752
Update cypress/e2e/timeline/timeline.spec.ts
luixxiul Mar 8, 2023
7ac5c6c
Merge branch 'develop' into fix-margin-list
luixxiul Mar 8, 2023
98adc11
Revert SCSS nesting
luixxiul Mar 8, 2023
3aef940
Merge the functions
luixxiul Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions cypress/e2e/timeline/timeline.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,96 @@ describe("Timeline", () => {
);
});

it("should render lists with paragraphs", () => {
cy.visit("/#/room/" + roomId);

// Wait until configuration is finished
cy.contains(
".mx_RoomView_body .mx_GenericEventListSummary .mx_GenericEventListSummary_summary",
"created and configured the room.",
).should("exist");

// Send unordered & ordered lists with paragraphs
cy.sendEvent(roomId, null, "m.room.message" as EventType, {
msgtype: "m.text" as MsgType,
body:
"Paragraph above lists\n\n" +
"- item 1\n- item 2\n- item 3\n\n" +
"Paragraph between lists\n\n" +
"1. item 1\n2. item 2\n3. item 3\n\n" +
"Paragraph below lists",
format: "org.matrix.custom.html",
formatted_body:
"<p>Paragraph above lists</p>\n" +
"<ul>\n<li>item 1</li>\n<li>item 2</li>\n<li>item 3</li>\n</ul>\n" +
"<p>Paragraph between lists</p>\n" +
"<ol>\n<li>item 1</li>\n<li>item 2</li>\n<li>item 3</li>\n</ol>\n" +
"<p>Paragraph below lists</p>\n",
});

// Confirm the paragraph below the lists was rendered
cy.get(".mx_EventTile[data-layout=group] .markdown-body p:last-of-type").should(
"have.text",
"Paragraph below lists",
);

// This is to check block margin of unordered & ordered lists
luixxiul marked this conversation as resolved.
Show resolved Hide resolved
const checkMarginList = () => {
cy.get("ul").should("have.css", "margin-block", "0px");
cy.get("ol").should("have.css", "margin-block", "0px");
};

// This is to check block margin of paragraphs around the lists except on compact modern layout
const checkMarginParagraphsWide = () => {
cy.get("p:first-of-type").should("have.css", "margin-block", "0px 16px"); // Paragraph above lists
cy.get("p:nth-of-type(2)").should("have.css", "margin-block", "16px"); // Paragraph between lists
cy.get("p:last-of-type").should("have.css", "margin-block", "16px 0px"); // Paragraph below lists
};
luixxiul marked this conversation as resolved.
Show resolved Hide resolved

// Check block margin values of paragraphs and lists on modern layout
cy.get(".mx_EventTile[data-layout=group] .markdown-body").within(() => {
checkMarginList();
checkMarginParagraphsWide();
});

// Exclude timestamp and read marker from snapshot
const percyCSS = ".mx_MessageTimestamp, .mx_RoomView_myReadMarker { visibility: hidden !important; }";
cy.get(".mx_MainSplit").percySnapshotElement("Lists with paragraphs on modern layout", { percyCSS });

// Check block margin values of paragraphs and lists on compact modern layout
cy.setSettingValue("useCompactLayout", null, SettingLevel.DEVICE, true);
cy.get(".mx_EventTile[data-layout=group] .markdown-body").within(() => {
cy.get("ul").should("have.css", "margin-block", "0px 4px");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If ul and ol margin is in fact not expected here, this needs to be replaced as well. https://github.com/matrix-org/matrix-react-sdk/pull/10305/files#r1127766737

cy.get("ol").should("have.css", "margin-block", "0px 4px");
cy.get("p:first-of-type").should("have.css", "margin-block", "0px 4px"); // Paragraph above lists
cy.get("p:nth-of-type(2)").should("have.css", "margin-block", "4px"); // Paragraph between lists
cy.get("p:last-of-type").should("have.css", "margin-block", "4px 0px"); // Paragraph below lists
});
cy.setSettingValue("useCompactLayout", null, SettingLevel.DEVICE, false);

cy.get(".mx_MainSplit").percySnapshotElement("Lists with paragraphs on compact modern layout", {
percyCSS,
});

// Check block margin values of paragraphs and lists on IRC layout
cy.setSettingValue("layout", null, SettingLevel.DEVICE, Layout.IRC);
cy.get(".mx_EventTile[data-layout=irc] .markdown-body").within(() => {
checkMarginList();
checkMarginParagraphsWide();
});

cy.get(".mx_MainSplit").percySnapshotElement("Lists with paragraphs on IRC layout", { percyCSS });

// Check block margin values of paragraphs and lists on bubble layout
cy.setSettingValue("layout", null, SettingLevel.DEVICE, Layout.Bubble);
cy.get(".mx_EventTile[data-layout=bubble] .markdown-body").within(() => {
checkMarginList();
checkMarginParagraphsWide();
});

cy.get(".mx_MainSplit").percySnapshotElement("Lists with paragraphs on bubble layout", { percyCSS });
});

it("should set inline start padding to a hidden event line", () => {
sendEvent(roomId);
cy.visit("/#/room/" + roomId);
Expand Down
38 changes: 26 additions & 12 deletions res/css/views/rooms/_EventTile.pcss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,32 @@ $left-gutter: 64px;
margin-top: 0;
Copy link
Member

Choose a reason for hiding this comment

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

(I don't really know why we need a margin-top here. As far as I can tell, it's the default anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me nether.

margin-bottom: 0;
}

/* EventTile on every layout */
.mx_EventTile & {
luixxiul marked this conversation as resolved.
Show resolved Hide resolved
/* Paragraph below ul or ol */
:is(ul, ol) + p {
margin-block: $spacing-16;
}
Copy link
Member

Choose a reason for hiding this comment

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

this seems to be directly overriding the settings at lines 647-650. Do we really need both settings?

Copy link
Contributor Author

@luixxiul luixxiul Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Are you suggesting that ul + p and ol + p would be covered by :is(blockquote > p, ol, ul) ?

Copy link
Member

Choose a reason for hiding this comment

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

Erm, I'm mostly asking the same question as in the review summary:

Generally: as I understand it, this works by setting a margin-top for <p> elements which follow a list. What is the reason for doing that, rather than setting a margin-bottom on lists, to bring them into line with the other block elements? I'm worried that with all these overrides, we're making the CSS very hard to reason about, and that there may be other situations that this won't fix.

}

/* EventTile on compact modern layout only */
.mx_MatrixChat_useCompactLayout .mx_EventTile[data-layout="group"] & {
luixxiul marked this conversation as resolved.
Show resolved Hide resolved
p,
ul,
ol,
dl,
blockquote,
pre,
table {
margin-bottom: $spacing-4; /* 1/4 of the non-compact margin-bottom */
}

/* Paragraph below ul or ol */
:is(ul, ol) + p {
margin-block: $spacing-4;
}
Copy link
Member

Choose a reason for hiding this comment

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

again, how does this interact with the settings at lines 647-650?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto

}
}
}

Expand Down Expand Up @@ -1316,18 +1342,6 @@ $left-gutter: 64px;
inset-block-start: -2rem;
}
}

.mx_EventTile_content .markdown-body {
p,
ul,
ol,
Copy link
Contributor Author

@luixxiul luixxiul Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure if the margin-bottom for ul and ol on compact modern layout has been expected, as the other layouts do not have margin-bottom for them.

dl,
blockquote,
pre,
table {
margin-bottom: $spacing-4; /* 1/4 of the non-compact margin-bottom */
}
}
}

&[data-shape="ThreadsList"][data-notification]::before,
Expand Down