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

Improvements for pie chart matrix #376

Closed
gcrabbe opened this issue Jun 7, 2024 · 2 comments · Fixed by #379
Closed

Improvements for pie chart matrix #376

gcrabbe opened this issue Jun 7, 2024 · 2 comments · Fixed by #379
Assignees

Comments

@gcrabbe
Copy link

gcrabbe commented Jun 7, 2024

I've just gotten around to testing the pie chart matrix feature introduced by #371. Before anything else, I'd like to thank you for your hard work. The functionality is almost exactly what I expected.

In my project, I have the following instance of item-piechart:

image

And its :matrix::

image

For reference, here is the item-matrix it replaced:

image

I'd like to make a few suggestions to make it even better (from most to least important):

  1. I'm missing a Result column. This has already been requested in item-matrix should support the same categories as item-piechart #368. Without it, it's not easy to identify which record is the failing one.
  2. I'd like the ability to set a title for each column. It's too bad the optional arguments to the :matrix: attribute have already been taken: they would have been perfect for this. If you're not willing to break compatibility over this, I guess we could add an :id_labels: argument.
  3. The subheaders are centered. Nothing else in the table is centered, but the subheaders are deliberately given a centered class. I feel like this should be left up to the theme (somehow).
  4. Could we make the pie chart background transparent? The white background does not play nice with the default RTD theme (c.f. the borders of the screenshot). I'm not (yet) asking for a dark-theme-compatible implementation, just something that won't stand out on the off-white background.
@JasperCraeghs
Copy link
Member

  1. I couldn't find an alternative in the CSS stylesheet of sphinx-rtd-theme. I want the subheader to be centered. Anything else makes less sense to me. I hope you can live with the current implementation.

Everything else has been implemented in #379

@gcrabbe
Copy link
Author

gcrabbe commented Jun 27, 2024

It's more of a "separation of content and presentation" concern. I just think the logic that builds the table should not have a say in how it looks.

I can always override the centering in CSS:

.pie-chart + .wy-table-responsive td.centered {
    text-align: left;
}

But I need to write a pretty specific filter to make sure I don't break centered in any other context.

But I would prefer if mlx.traceability just used class="head" to signal a heading (it is a heading, after all) instead of forcing it with a class="centered" and a <strong> tag.

Then, in your own CSS, you could center all table headings:

.wy-table-responsive .head {
    text-align: center;
}

Only subheadings:

.wy-table-responsive td.head {
    text-align: center;
}

Or only cells spanning multiple columns:

.wy-table-responsive td[colspan]:not([colspan="1"]) {
    text-align: center;
}

However, as I've said, it's a really minor detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants