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

Use auto table-layout on desktop #1601

Merged
merged 3 commits into from Jul 20, 2023

Conversation

not-my-profile
Copy link
Contributor

@not-my-profile not-my-profile commented Jul 13, 2023

On desktop the tables in the specification currently have quite awkward column widths, making them harder to read.

Before After
image image

While the fixed table-layout does have an advantage on mobile since it avoids unnecessary horizontal scrolling in the tables, we can just use it on narrow browser widths via a media query, while still using the more readable auto layout on desktop.

Preview: https://pr1601--matrix-spec-previews.netlify.app

@not-my-profile not-my-profile requested a review from a team as a code owner July 13, 2023 12:18
Comment on lines +364 to +367
* Thus, we override the "display" property here. This may no longer be necessary once
* Docsy updates to Bootstrap v5+: https://github.com/google/docsy/issues/470.
* For more details, see
* https://github.com/matrix-org/matrix-spec/pull/1295/files#r1010759688 */
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should update our fork of docsy now that google/docsy#470 is closed...

Anyway, I guess that is a separate problem.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Thanks! Please add a changelog and sign-off.

@not-my-profile
Copy link
Contributor Author

Just signed-off the commit. I'd be happy to add a changelog entry but none of the allowed changelog entry types fits this PR (new, feature, clarification, breaking, deprecation). I'd classify this as "style" but I'm assuming scripts/check-newsfragments won't let that pass.

@richvdh
Copy link
Member

richvdh commented Jul 19, 2023

Thanks! You could make it a clarification in the internal section.

@not-my-profile
Copy link
Contributor Author

CONTRIBUTING.rst defines clarification as:

clarification - Used when an area of the spec is being improved upon and does not change or introduce any functionality.

That really sounds like a change to the text of the spec ... so I don't really want to classify it as "clarification" since I'd consider that as misleading.

@richvdh
Copy link
Member

richvdh commented Jul 19, 2023

I mean you're right, and #1369 mentions the fact that it's not a great match. But it's what we use for now.

Compare https://spec.matrix.org/v1.7/changelog/#internal-changestooling for where we've done this in the past. None of those are really changes to the spec.

Signed-off-by: Martin Fischer <martin@push-f.com>
@not-my-profile
Copy link
Contributor Author

Fair enough ... added the newsfragment.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Thank you!

(Aside: it's fine in this case, because the changes are relatively small, but please don't force-push after a review: it makes it hard to see what has changed since the previous review. We can clean up the commit history when we merge the PR.)

assets/scss/custom.scss Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
Improve the layout of tables on desktop.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Improve the layout of tables on desktop.
Improve the layout of tables on desktop displays. Contributed by @not-my-profile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really like my work being attributed to my GitHub username :/

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry. I can remove this.

Copy link
Member

Choose a reason for hiding this comment

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

would you prefer an alternative attribution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah: Martin Fischer.

Copy link
Member

Choose a reason for hiding this comment

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

Done: f5035b8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks <3

assets/scss/custom.scss Outdated Show resolved Hide resolved
@richvdh richvdh enabled auto-merge (squash) July 20, 2023 07:22
@richvdh richvdh merged commit 1a11a7b into matrix-org:main Jul 20, 2023
10 checks passed
richvdh added a commit that referenced this pull request Jul 20, 2023
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.

None yet

2 participants