Skip to content

Conversation

@cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Nov 6, 2023

Fixes #11791. I made the text a bit lighter (60% gray), which makes it easier to read in dark mode. It should be still quite readable in light mode as well.

Old (current) light:
light-old

Old (current) dark:
dark-old

New light:
light

New dark:
dark

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

I like the choice of color, but [nitpick] the diff is quite large, because of editing via inkscape. Could you instead tweak the script that generates the logo? I think it's this line:

tag_patch = PathPatch(tag_clip, facecolor="k", edgecolor="none", zorder=10)

@hoechenberger
Copy link
Member

Looks very nice!!!

@hoechenberger
Copy link
Member

Actually. Shouldn't we get rid of the tagline. Or change it: Brain analysis ... or something like that?

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 7, 2023

I cannot generate the updated logo:

findfont: Generic family 'sans-serif' not found because none of the following families were found: Primetime
/Users/clemens/Projects/mne-python/logo/generate_mne_logos.py:111: MatplotlibDeprecationWarning: The collections attribute was deprecated in Matplotlib 3.8 and will be removed two minor releases later.
  for coll in cs.collections:
findfont: Generic family 'sans-serif' not found because none of the following families were found: Cooper Hewitt
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
File ~/Projects/mne-python/logo/generate_mne_logos.py:181
    178 patch.remove()
    180 # modify to make an icon
--> 181 ax.patches.pop(-1)  # no tag line for our icon
    182 patch = Ellipse(xy, r, r, clip_on=False, zorder=-1, fc="k")
    183 ax.add_patch(patch)

AttributeError: 'ArtistList' object has no attribute 'pop'

@cbrnr cbrnr marked this pull request as draft November 7, 2023 05:56
@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 7, 2023

And yes, I did install the font.

@hoechenberger
Copy link
Member

Maybe you need to refresh the MPL font cache?

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 7, 2023

Thanks, that was it!

Now I saw that the logo also needs the "Primetime" font, which is only free for personal use. I'm not sure that our logo qualifies for personal use.

Edit: I'm now using "Cooper Hewitt" everywhere.

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 7, 2023

Replacing the Primetime font is not a good idea, the PNGs look terrible...

@larsoner
Copy link
Member

larsoner commented Nov 7, 2023

The image diffs look like they're headed in a good direction! A couple of things I noticed though:

  1. The main logo tagline gets harder to read when shrunk (at least on GH diff view):

    Screenshot from 2023-11-07 11-14-31

  2. mne/icons/mne_default_icon.png and mne/icons/mne_splash.png which are a bit off:

    Screenshot from 2023-11-07 11-15-49

Let me know if you want help. My first/naive thought is that maybe we don't need center_fudge anymore at least for the icon and splash, but the white outline is also off so there is probably more to it than that.

Now I saw that the logo also needs the "Primetime" font, which is only free for personal use... I'm now using "Cooper Hewitt" everywhere.

Makes sense to switch if that is indeed the case. @drammock okay with the switch away from PrimeTime? I think it looks fine/comparable to me but IIRC you chose the fonts originally

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 7, 2023

@larsoner please feel free to take over! Regarding your first point, I think you need to look at it with white background, because it get's hard to read because of the checkered background IMO.

Also, the SVG diffs are still massive even though they were generated with the script (but apparently, a lot has changed in how MPL renders SVGs?). But I guess that's OK.

@drammock
Copy link
Member

drammock commented Nov 7, 2023

Now I saw that the logo also needs the "Primetime" font, which is only free for personal use.

from the designer's website:

With a free for Personal use version, you can't use PRIMETIME © for a commercial purpose.

MNE is not commercial software, so as I said long ago as long as we're not fundraising, selling the stickers, or start charging for MNE I think we're in the clear.

@drammock okay with the switch away from PrimeTime? I think it looks fine/comparable to me but IIRC you chose the fonts originally

I chose CooperHewitt but I think @agramfort chose Primetime (and I recall being discouraged from changing it when I was scripting the logo generation because "Alex likes Primetime"). Anyway I don't think we need to change it, see above.

@hoechenberger
Copy link
Member

Let's not touch Alex's Primetime then 🎉🍹

@larsoner
Copy link
Member

larsoner commented Nov 7, 2023

Okay I think I'm happy now. @drammock @cbrnr feel free to look -- GitHub has a nice image diff nowadays! Changes:

  1. Tweaked tagline the weight upward slightly -- I've noticed especially on HiDPI screens (my mac) that the Light font is difficult to read.
  2. Added a little sanity check that matplotlib probably/hopefully finds the correct font. On my system I had several weights and it always used Thin even though I had thin/light/book/medium installed and I think it was supposed to pick Light. So I had to rm the other variants then fc-cache update and rerun. Now I have it using Book.
  3. To offset this, I moved from black to "0.2" and from white to "0.8" which I think helps reduce the prominence close to what it was before without sacrificing readability on HiDPI displays.
  4. Added mne_logo_gray.svg that lives in doc/_static. Once it deploys tonight we can change our README.rst to use mne.tools/dev/_static/mne_logo_gray.svg.

@larsoner
Copy link
Member

larsoner commented Nov 7, 2023

Okay I'll actually stop pushing commits now!

@larsoner larsoner added this to the 1.6 milestone Nov 7, 2023
@drammock
Copy link
Member

drammock commented Nov 7, 2023

@larsoner OK by me if you merge when happy

from sphinx.util.display import status_iterator
except Exception:
from sphinx.util import status_iterator
root = Path(__file__).parent.parent.parent.absolute()
Copy link
Member

Choose a reason for hiding this comment

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

Even on this branch:

$ git grep "\.parent\.parent" | wc -l
70

but they are now at least all isolated to tests files, I made the others more compact in this PR

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 7, 2023

Wait, why am I seeing this on my phone in dark mode (README.rst on main):

Screenshot_2023-11-07-22-00-58-899_com.github.android.jpg

@larsoner
Copy link
Member

larsoner commented Nov 7, 2023

No idea, I can't replicate on my mobile or on macOS in inspection mode as an iPhone, or in Safari

@hoechenberger
Copy link
Member

You mean that gray bar underneath the tagline?

@larsoner larsoner merged commit ffbce01 into mne-tools:main Nov 7, 2023
@hoechenberger
Copy link
Member

Looks like this for me now in the browser and GH app:

image

image

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 8, 2023

You mean that gray bar underneath the tagline?

No, I meant the white (gray?) text, which is 100% readable (on main). But apparently, this is a dark mode feature on my Android phone, because I cannot replicate on my Mac. Still, pretty neat!

@cbrnr cbrnr deleted the tweak-logo branch November 8, 2023 06:04
@hoechenberger
Copy link
Member

I cannot read the tagline in dark mode 🥲 (see my screenshots above)

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 8, 2023

Yes, I think it's something my phone does.

@hoechenberger
Copy link
Member

Yeah but … wasn't that something this PR aimed to address too? 😅

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 8, 2023

Yes, but as @larsoner mentioned here, we merged this prematurely. I'm not entirely sure what the plan is, but he created a third logo variant mne_logo_gray.svg, which I think we need to include in README.rst. Previously, I modified mne_logo.svg directly, which is the logo currently included in README.rst.

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 8, 2023

#12184

@hoechenberger
Copy link
Member

Ah okay! I missed that!

larsoner added a commit to pablomainar/mne-python that referenced this pull request Nov 8, 2023
* upstream/main:
  BUG: Fix bug with spectrum warning (mne-tools#12186)
  Add argument splash to disable splash-screen from Qt-browser (mne-tools#12185)
  BUG: Fix bug with logging and n_jobs>1 (mne-tools#12154)
  Use gray logo (works in light and dark modes) (mne-tools#12184)
  Tweak logo for dark mode (mne-tools#12176)
  ENH: Improve Covariance.__repr__ (mne-tools#12181)
  ENH: Enable sensor-specific OPM coregistration in mne coreg (mne-tools#11405)
  Tweak README.rst (mne-tools#12166)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12177)
  MAINT: Add branch coverage (mne-tools#12174)
  OpenSSF (mne-tools#12175)
  fix docstring in 60_sleep.py (mne-tools#12171)
  FIX: skip empty lines in read_raw_eyelink (mne-tools#12172)
  FIX: Fix bug with coreg scalars (mne-tools#12164)
  Changed casting rule in np.clip to allow reading of raw GDF files (mne-tools#12168)
  [DOC] Add documentation for setting montage order (mne-tools#12160)
  Fix inferring fiducials from EEGLAB (mne-tools#12165)
larsoner added a commit to hoechenberger/mne-python that referenced this pull request Nov 8, 2023
…o-pyproject.toml

* upstream/main:
  MAINT: Fix CIs (mne-tools#12188)
  BUG: Fix bug with default alpha and axes (mne-tools#12187)
  BUG: Fix bug with spectrum warning (mne-tools#12186)
  Add argument splash to disable splash-screen from Qt-browser (mne-tools#12185)
  BUG: Fix bug with logging and n_jobs>1 (mne-tools#12154)
  Use gray logo (works in light and dark modes) (mne-tools#12184)
  Tweak logo for dark mode (mne-tools#12176)
  ENH: Improve Covariance.__repr__ (mne-tools#12181)
  ENH: Enable sensor-specific OPM coregistration in mne coreg (mne-tools#11405)
  Tweak README.rst (mne-tools#12166)
@hoechenberger
Copy link
Member

hoechenberger commented Nov 9, 2023

Only slightly related but I thought it was funny:
imageimageimageimage

snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
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.

mne_logo.svg bottom text hard to read in dark mode

4 participants