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.

Sign up for GitHub

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

Jump to bottom

Improve feature profile summary table styling #1599

Closed
wants to merge 1 commit into from
Closed

Improve feature profile summary table styling #1599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 21 additions & 0 deletions assets/scss/custom.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,3 +512,24 @@ dd {
border-radius: 0.25rem; // was $border-radius, but that var isn't accessible here.
}
}

/* Styling for the summary-table shortcode */
.summary-table {
Copy link
Member

Choose a reason for hiding this comment

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

I think this makes the !important on the padding unnecessary:

Suggested change
.summary-table {
table.summary-table {

tr {
// Unset the .table-striped styling that docsy applies by default.
background-color: unset !important;
}
.required {
background-color: rgba($black, 0.07);
position: absolute;
top: 0;
bottom: 0;
left: 0;
right: 0;
Comment on lines +524 to +528
Copy link
Member

Choose a reason for hiding this comment

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

Some comments would be nice.

Suggested change
position: absolute;
top: 0;
bottom: 0;
left: 0;
right: 0;
// expand to fill the entire table cell
position: absolute;
top: 0;
bottom: 0;
left: 0;
right: 0;

z-index: -1;
Copy link
Member

Choose a reason for hiding this comment

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

why is this required?

}
td {
position: relative;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is something to do with the span resizing? comment it, please

padding: .3rem 0.75rem !important;
Copy link
Member

Choose a reason for hiding this comment

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

Comment, please:

Suggested change
padding: .3rem 0.75rem !important;
// use a more compact table layout than the default.
padding: .3rem 0.75rem !important;

}
}
2 changes: 2 additions & 0 deletions config.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ weight = 1
[markup.goldmark.renderer]
# Enables us to render raw HTML
unsafe = true
[markup.goldmark.parser.attribute]
block = true # used by the summary-table shortcode
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice to give a quick example of what it actually does. And/or link to the documentation

[markup.highlight]
# See a complete list of available styles at https://xyproto.github.io/splash/docs/all.html
# If the style is changed, remember to regenerate the CSS with:
Expand Down
2 changes: 2 additions & 0 deletions content/client-server-api/_index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2565,6 +2565,7 @@ that profile.

#### Summary

{{<summary-table>}}
Copy link
Member

Choose a reason for hiding this comment

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

Could we use {{% summary-table %}} so that we don't have to call markdownify explicitly?

| Module / Profile | Web | Mobile | Desktop | CLI | Embedded |
|------------------------------------------------------------|-----------|----------|----------|----------|----------|
| [Instant Messaging](#instant-messaging) | Required | Required | Required | Required | Optional |
Expand Down Expand Up @@ -2603,6 +2604,7 @@ that profile.
| [Event Annotations and reactions](#event-annotations-and-reactions) | Optional | Optional | Optional | Optional | Optional |
| [Threading](#threading) | Optional | Optional | Optional | Optional | Optional |
| [Reference Relations](#reference-relations) | Optional | Optional | Optional | Optional | Optional |
{{</summary-table>}}

*Please see each module for more details on what clients need to
implement.*
Expand Down
6 changes: 6 additions & 0 deletions layouts/shortcodes/summary-table.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{{/*

This template is used to visually emphasize table cells containing the text "Required".
Copy link
Member

Choose a reason for hiding this comment

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

Please could you add some more words about how it is intended to be used? Is it for all tables which summarise something, or for a specific summary table? What is the input (a markdown table, I think?), and how is the output returned (it outputs an HTML table ?)

Copy link
Member

Choose a reason for hiding this comment

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

I note it also adds a summary-table CSS class to the table, which should be mentioned.


*/}}
{{ print (replace .Inner "Required" "<span class=required></span>Required") "{.summary-table}" | markdownify }}
Copy link
Member

Choose a reason for hiding this comment

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

why is the print necessary here? isn't the result of {{ }} written to the output anyway?

Loading