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

upsize label sizing in bar/line charts #1576

Merged
merged 8 commits into from
Nov 7, 2021
Merged

Conversation

jchorl
Copy link
Contributor

@jchorl jchorl commented Nov 6, 2021

Just upsizes some label sizing in the bar/line charts. I understand there is no optimal size for everyone. However I find 8 point font very small, and can't imagine many folks would complain with slightly bigger font sizing. Thoughts?

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md has been updated

@ewels
Copy link
Member

ewels commented Nov 7, 2021

This one might be more complex than it first seems. It was a looong time ago that I wrote this code, but I seem to remember that I spent quite a long time fighting with margins and balancing font size versus spacing for long sample names.

It's very common for sample names to be huge in MultiQC reports, so this needs to work in those cases.

@jchorl
Copy link
Contributor Author

jchorl commented Nov 7, 2021

Any fud with shoving it in the pconfig so we can e.g. pconfig.get("labelSize", 8)?

@ewels
Copy link
Member

ewels commented Nov 7, 2021

Not at all - that follows my general ethos which is "sensible defaults with ability to customise".

@ewels
Copy link
Member

ewels commented Nov 7, 2021

Testing your PR and comparing before / after, it could also be as simple as me trying to match the matplotlib font sizes to the normal interactive plot font sizes / HTML text font sizes.

I'd be willing to meet you half way and set the defaults to 10, how does that sound? Also making it configurable sounds like a sensible thing to add as well.

@jchorl
Copy link
Contributor Author

jchorl commented Nov 7, 2021

I'll just make it configurable - then we don't have to worry about catering to personal prefs in defaults

@jchorl
Copy link
Contributor Author

jchorl commented Nov 7, 2021

Tested using https://raw.githubusercontent.com/ewels/MultiQC_TestData/master/data/custom_content/embedded_config/test_bargraph_mqc.txt

With/without:

# pconfig:
#    omitted fields...
#    labelSize: 18

Screenshot 2021-11-07 at 22-00-27 MultiQC Report
Screenshot 2021-11-07 at 21-59-53 MultiQC Report

I didn't touch the js plotly stuff. That code looks a little hairy. Let me know if that's fine, and also if this config should be documented somewhere. I couldn't find an enumeration of all the possible pconfig overrides to add this to.

@ewels
Copy link
Member

ewels commented Nov 7, 2021

Nice, thanks! If you could just add some docs about this, then good to merge.. 👍🏻

@jchorl
Copy link
Contributor Author

jchorl commented Nov 7, 2021

Wasn't sure where to put docs. Let me know if another place is better!

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻 Changelog too, but I can do that myself in a second..

docs/plots.md Outdated Show resolved Hide resolved
docs/plots.md Outdated Show resolved Hide resolved
@ewels ewels merged commit 02a9af4 into MultiQC:master Nov 7, 2021
ewels added a commit that referenced this pull request Nov 7, 2021
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.

2 participants