Improve feature profile summary table styling #1599
Conversation
The 'Feature Profiles' section of the 'Client-Server API' specification contains a table, showing whether a module is required or optional for every profile. Previously the table was hard to read since "Required" looks visually very similar to "Optional". This commit improves the readability of the table by automatically applying a darker background color to table cells containing "Required" via a new Hugo shortcode. Signed-off-by: Martin Fischer <martin@push-f.com>
not-my-profile
force-pushed
the
improve-summary-table-styling
branch
from
831eb3e
to
608818a
Compare
There was a problem hiding this comment.
Thank you for the contribution.
It seems quite complicated, though. Could we achieve much the same effect rather more simply just by making the "Required" text bold ?
Please do make sure that your changes are well-commented to help future maintainers understand. I've made some suggestions.
| @@ -0,0 +1,6 @@ | |||
| {{/* | |||
|
|
|||
| This template is used to visually emphasize table cells containing the text "Required". | |||
There was a problem hiding this comment.
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 ?)
There was a problem hiding this comment.
I note it also adds a summary-table CSS class to the table, which should be mentioned.
| This template is used to visually emphasize table cells containing the text "Required". | ||
|
|
||
| */}} | ||
| {{ print (replace .Inner "Required" "<span class=required></span>Required") "{.summary-table}" | markdownify }} |
There was a problem hiding this comment.
why is the print necessary here? isn't the result of {{ }} written to the output anyway?
| @@ -2565,6 +2565,7 @@ that profile. | |||
|
|
|||
| #### Summary | |||
|
|
|||
| {{<summary-table>}} | |||
There was a problem hiding this comment.
Could we use {{% summary-table %}} so that we don't have to call markdownify explicitly?
| position: absolute; | ||
| top: 0; | ||
| bottom: 0; | ||
| left: 0; | ||
| right: 0; |
There was a problem hiding this comment.
Some comments would be nice.
| 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; |
| bottom: 0; | ||
| left: 0; | ||
| right: 0; | ||
| z-index: -1; |
| z-index: -1; | ||
| } | ||
| td { | ||
| position: relative; |
There was a problem hiding this comment.
I think this is something to do with the span resizing? comment it, please
| @@ -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 { | |||
There was a problem hiding this comment.
I think this makes the !important on the padding unnecessary:
| .summary-table { | |
| table.summary-table { |
| } | ||
| td { | ||
| position: relative; | ||
| padding: .3rem 0.75rem !important; |
There was a problem hiding this comment.
Comment, please:
| padding: .3rem 0.75rem !important; | |
| // use a more compact table layout than the default. | |
| padding: .3rem 0.75rem !important; |
| @@ -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 | |||
There was a problem hiding this comment.
it would be nice to give a quick example of what it actually does. And/or link to the documentation
|
Please also take a look at the contributing guide, particularly with respect to changelog entries Edit: I see you have signed this off, thank you! Please add a changelog entry though. |
|
@not-my-profile are you still interested in working on this? |
|
Sorry that I haven't replied. Thanks for the review ... your feedback is very reasonable and appreciated. I unfortunately am currently busy with other projects. I might circle back to this at some point in the future but can't tell you when. |
|
@not-my-profile understood, and no need to apologise. If it's unlikely you'll be working on this in the near future, then rather than keeping this PR open indefinitely, I'll go ahead and close it for now. That's not to say we're not interested in the work, and if you get a chance to pick it up again, do give us a ping and we'll be happy to reopen it! Thanks for the contribution in any case. |
This PR improves the styling of the feature profile summary table in the Client-Server API specification:
Preview: https://pr1599--matrix-spec-previews.netlify.app/client-server-api/#summary