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

Improve HTML formatting #82

Closed
wants to merge 1 commit into from
Closed

Improve HTML formatting #82

wants to merge 1 commit into from

Conversation

akaszynski
Copy link
Collaborator

@akaszynski akaszynski commented Jun 25, 2022

Resolve #62 by removing the hard-coded table color.

Improves HTML report by forcing two columns to make the package listing more readable.

image

@prisae
Copy link
Collaborator

prisae commented Jun 25, 2022

I like the colours, thanks @akaszynski

I would be a bit more cautious personally for the two-column change. It is quite a change. Could it be a keyword-change instead, eg scooby.Report(twocolumn={False}/True). (Just that I personally quite like the multi-col style, so I'd prefer to keep it.)

@@ -192,7 +195,7 @@ def __init__(
optional = ['numpy', 'scipy', 'IPython', 'matplotlib', 'scooby']

PythonInfo.__init__(self, additional=additional, core=core, optional=optional, sort=sort)
self.ncol = int(ncol)
self.ncol = 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think ncol should be hardcoded here, doesn't render this the ncol input parameter useless?

@@ -269,59 +272,43 @@ def colspan(html, txt, ncol, nrow):
elif nrow % 2 == 0:
html += "background-color: #ddd;"
html += border + " colspan='"
html += str(2 * ncol) + "'>%s</td>\n" % txt
html += f"{ncol}'>{txt}</td>\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes the ncol behaviour. ncol before meant than in a column of ncol there was one column for package names and and one column for version numbers. Now it means that ncol has to account for package names and version numbers separately.

@prisae
Copy link
Collaborator

prisae commented Jun 27, 2022

Could we split your two points, (1) remove hard-coded table color and (2) force two columns, into two separate PR's? I think (1) would be just ready to go, whereas (2) I am personally against, or it would need at least some discussion and thinking and agreeing IMHO.

This was referenced Jul 22, 2022
@prisae
Copy link
Collaborator

prisae commented Jul 23, 2022

Is it OK to close this one @akaszynski in favour of #88 and potentially #89 ?

@akaszynski
Copy link
Collaborator Author

Is it OK to close this one @akaszynski in favour of #88 and potentially #89 ?

Absolutely. #88 is better.

@akaszynski akaszynski closed this Jul 23, 2022
@akaszynski akaszynski deleted the feat/dark_theme branch July 23, 2022 15:26
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.

Color scheme in Jupyter Darkmode
2 participants