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

[FIX] Report Table: Make Table Headers Bold #2668

Merged
merged 1 commit into from
Oct 11, 2017

Conversation

nikicc
Copy link
Contributor

@nikicc nikicc commented Oct 10, 2017

Issue

When reporting a view (or model?) through report_table header column and row is printed in the same format as the data values. Although there is a code that mark header columns as <th> (and not as <td>) this is invalidated by the font-weight attribute.

E.g. in Test & Score before:

screen shot 2017-10-10 at 16 33 43
and after:
screen shot 2017-10-10 at 16 33 07

Description of changes

Set font-weight to bold when inside header so <th> is not invalidated.

Includes
  • Code changes
  • Tests
  • Documentation

Copy link
Contributor

@kernc kernc left a comment

Choose a reason for hiding this comment

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

Default HTML style sheets must have changed. I'm sure headers were bold before.

@janezd
Copy link
Contributor

janezd commented Oct 10, 2017

I like the change. But what if we changed the style definition for the th tag instead? The tag is used in several places and I suppose that we'd always like to have it bold.

@codecov-io
Copy link

codecov-io commented Oct 10, 2017

Codecov Report

Merging #2668 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2668   +/-   ##
=======================================
  Coverage   75.51%   75.51%           
=======================================
  Files         332      332           
  Lines       58014    58014           
=======================================
  Hits        43812    43812           
  Misses      14202    14202

@nikicc
Copy link
Contributor Author

nikicc commented Oct 10, 2017

I like the change. But what if we changed the style definition for the th tag instead? The tag is used in several places and I suppose that we'd always like to have it bold.

I agree. Please, check again.

@janezd
Copy link
Contributor

janezd commented Oct 10, 2017

Could we avoid !important? I think the culprit is (report.py, line 244)

weight = 'bold' if font and font.bold() else 'normal'.

What if we had

weight = 'font-weight: bold;' if font and font.bold() else ''

and then replace 'font-weight:{weight};' with {weight}? That is, skip font-weight property when it is not bold?

Another benefit from this would be that our HTML would look a small tad less like it was generated by Word. :)

@nikicc
Copy link
Contributor Author

nikicc commented Oct 11, 2017

Fixed, please check again.

@janezd janezd merged commit 6f303b7 into biolab:master Oct 11, 2017
@nikicc nikicc deleted the fix-header-in-report-views branch October 11, 2017 18:36
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.

5 participants