fix(markdown): allow horizontal scrolling in nested HTML tables #3466
Conversation
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3466/ |
adrianschmidt
force-pushed
the
markdown-nested-html-tables
branch
from
907b7fd to
da87572
Compare
WalkthroughThe pull request updates the SCSS styling for tables. It introduces a new Changes
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/markdown/partial-styles/_tables.scss(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Build
🔇 Additional comments (2)
src/components/markdown/partial-styles/_tables.scss (2)
10-17: Table Block Styling within the Host Scoped Rule
The table styling defined within the:host(limel-markdown:not(.no-table-styles))selector is clear and consistent. The properties (e.g.,table-layout: auto,min-width: 100%, and margin settings) ensure that tables inherit a flexible layout inside the component.
64-64: Closing the :host Block Correctly
The closing brace at line 64 correctly terminates the style block for the:host(limel-markdown:not(.no-table-styles))rule. No issues noted here.
| tbody { | ||
| border: 1px solid rgb(var(--contrast-400)); | ||
| border-radius: 0.25rem; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
New tbody Styling: Border and Border-Radius Addition
The addition of a tbody block with a border and border-radius enhances the visual separation of the table body. However, please verify that applying border-radius on the tbody element produces consistent results across all target browsers and email clients, as some browsers may handle border-radius on table sections differently.
If needed, I can help set up cross-browser tests to validate this styling.
| table { | ||
| // This enables very wide tables to be scrollable on small | ||
| // containers. However, since the default is `display: table`, | ||
| // these styles could potentially create other misalignment or | ||
| // layout issues. For example, small tables won't be 100% wide | ||
| // anymore. I have accepted this tradeoff and have not yet | ||
| // found anything too horrible or better solution. /Kia | ||
| display: block; | ||
| box-sizing: border-box; | ||
| overflow-x: auto; | ||
| max-width: 100%; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enabling Horizontal Scrolling for Tables Outside the Host
The new table block defined from lines 65 to 76 successfully implements horizontal scrolling for wide tables by using display: block, overflow-x: auto, and max-width: 100%. The explanatory comments clearly describe the tradeoffs involved.
For enhanced mobile support (especially on iOS), consider adding the -webkit-overflow-scrolling: touch; property to enable smoother scrolling. For example:
- overflow-x: auto;
+ overflow-x: auto;
+ -webkit-overflow-scrolling: touch;This change can improve the overall user experience on mobile devices.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| table { | |
| // This enables very wide tables to be scrollable on small | |
| // containers. However, since the default is `display: table`, | |
| // these styles could potentially create other misalignment or | |
| // layout issues. For example, small tables won't be 100% wide | |
| // anymore. I have accepted this tradeoff and have not yet | |
| // found anything too horrible or better solution. /Kia | |
| display: block; | |
| box-sizing: border-box; | |
| overflow-x: auto; | |
| max-width: 100%; | |
| } | |
| table { | |
| // This enables very wide tables to be scrollable on small | |
| // containers. However, since the default is `display: table`, | |
| // these styles could potentially create other misalignment or | |
| // layout issues. For example, small tables won't be 100% wide | |
| // anymore. I have accepted this tradeoff and have not yet | |
| // found anything too horrible or better solution. /Kia | |
| display: block; | |
| box-sizing: border-box; | |
| overflow-x: auto; | |
| -webkit-overflow-scrolling: touch; | |
| max-width: 100%; | |
| } |
test a big table
Paste this MD code in a markdown example:
fix https://github.com/Lundalogik/limepkg-email/issues/1303
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile:
Summary by CodeRabbit