Skip to content

Fix layout and horizontal scrollable tables #772

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

Merged
merged 4 commits into from
Mar 19, 2025

Conversation

reakaleek
Copy link
Member

@reakaleek reakaleek commented Mar 18, 2025

Context

The content overflows if there is a table that is wider than the main content column.

Details

Fixes the broken layout caused by wide tables which at the same time fixes the horizontal scrolling of tables at certain widths.

Previous behaviour

broken-layout-1742342218347.mp4

Fixed behaviour

fixed-layout-1742342114676.mp4

@reakaleek reakaleek marked this pull request as ready for review March 19, 2025 00:00
@Copilot Copilot AI review requested due to automatic review settings March 19, 2025 00:00
@reakaleek reakaleek requested a review from a team as a code owner March 19, 2025 00:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes layout issues and improves horizontal scrolling for wide tables. It updates the hx-select-oob selectors to include "#toc-nav" in both the application code and tests, and adjusts layout components to use a grid-based structure for improved responsiveness.

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/Elastic.Markdown/Helpers/Htmx.cs Appended "#toc-nav" to the hx-select-oob selector to capture additional content.
tests/Elastic.Markdown.Tests/Inline/* Updated expected HTML output to include "#toc-nav" for consistency.
src/Elastic.Markdown/Slices/Layout/_PagesNav.cshtml Modified sidebar width class from fixed width to auto for responsive behavior.
src/Elastic.Markdown/Slices/Layout/_TableOfContents.cshtml Removed fixed width class to allow flexible layout.
src/Elastic.Markdown/Slices/Layout/_TocTree.cshtml Included "#toc-nav" in hx-select-oob to align with the revised navigation handling.
src/Elastic.Markdown/Slices/_Layout.cshtml Converted the layout container to a grid layout and added an overflow-x-hidden class to fix horizontal scrolling issues.
Files not reviewed (1)
  • src/Elastic.Markdown/Assets/markdown/table.css: Language not supported
Comments suppressed due to low confidence (2)

src/Elastic.Markdown/Slices/_Layout.cshtml:16

  • Ensure that the new grid layout structure and the removal of the extra closing correctly preserves the intended HTML nesting and layout behavior across all viewports.
<div class="grid grid-cols-1 md:grid-cols-[240px_auto] lg:grid-cols-[1fr_3fr_1fr] gap-4 container">

src/Elastic.Markdown/Slices/Layout/_TableOfContents.cshtml:2

  • Verify that removing the fixed width class (w-60) does not adversely affect the design consistency or spacing of the table of contents in larger layouts.
<aside class="sidebar hidden lg:block order-3 border-l-1 border-l-grey-20">

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

I like that the full window now resizes down correctly.

I dont think the table should be allowed to extend beyond its containers max width. E.g we now need to horizontally scroll to see the table as it extends beyond the max width of it's containers (2nd video).

@reakaleek
Copy link
Member Author

reakaleek commented Mar 19, 2025

I like that the full window now resizes down correctly.

I dont think the table should be allowed to extend beyond its containers max width. E.g we now need to horizontally scroll to see the table as it extends beyond the max width of it's containers (2nd video).

The horizontal scroll was actually implemented before also. It was just broken at certain widths.

The main use case for it is the mobile view, which is a common responsive design pattern.

Thank you for the approval. I will create a GH discussion to discuss exactly this and and a possible strategy to handle large tables in the future.

@reakaleek reakaleek added the fix label Mar 19, 2025
@reakaleek
Copy link
Member Author

#773

@reakaleek reakaleek merged commit f4c005e into main Mar 19, 2025
10 of 11 checks passed
@reakaleek reakaleek deleted the feature/fix-layout-and-tables branch March 19, 2025 08:53
Mpdreamz pushed a commit that referenced this pull request Mar 19, 2025
* Fix layout and horizontal scrollable tables

mend

* Use spacing var

* Add custom scrollbar

* tweaks
Mpdreamz added a commit that referenced this pull request Mar 19, 2025
* Add global navigation and improve diagnostic handling

This commit introduces a GlobalNavigation feature, parsing the new `navigation.yml` file to handle global navigation references. It also replaces `BuildContext` with `DiagnosticsCollector` for diagnostics, ensuring clearer separation of concerns. Additionally, it includes new tests and updates project files for better testing and navigation handling.

* Use navigation.yml to control url path prefixes for resolving

* path_prefix is mandatory except for top level docs-content references

* stage commit

* stage commit

* File copy now in a decent place

* temporary home for in flux paths

* fix failing test

* remove not referenced test project

* Restrict docset.yml configs that define toc.yml sections to ONLY link to sub toc.yml files (#767)

* Restrict docset.yml configs that define toc.yml sections to ONLY link to sub toc.yml files

* relax check for narrative docs too, plays by different rules

* dotnet format

* reference Project directly

* Fix layout and horizontal scrollable tables (#772)

* Fix layout and horizontal scrollable tables

mend

* Use spacing var

* Add custom scrollbar

* tweaks

* Add table of contents scope management and refactor diagnostics (#774)

* Add table of contents scope management and refactor diagnostics

Introduced `ITableOfContentsScope` to manage scope boundaries, improving link validation and table of contents processing. Updated diagnostics to emit hints for images outside the ToC scope and refactored functions for better reusability. Adjusted related tests and internal structures accordingly.

* Add TODO to make this an error

* post merge build fixes

* good enough state

---------

Co-authored-by: Jan Calanog <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants