Skip to content

Conversation

@cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Nov 3, 2023

I've tried to tweak our README a little to make it easier to see what's important.

I did not touch the badges at the very top, but I would strongly suggest that we restrict ourselves to only one line (as rendered on GitHub). This means that at least one badge has to go. IMO, only the first three badges are important (PyPI, conda, DOI), the other four don't really contain super important information that needs to be presented at the top of the README (we already describe the forum in the document, so no need for this badge anyway).

To Do:

  • Remove weird underlined space in badge table (need help from someone who actually knows rST).
  • Streamline badge texts (e.g. always start with uppercase letter, etc.)
Screenshot 2023-11-04 at 17 03 00

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

if we need to remove a badge, then I'd remove the "bandit" one.

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.

The two lines of badges don't bother me, but I wouldn't mind if the badges moved down below the big logo. While you're messing with the README there's also #11791 if you want to tackle that here

@hoechenberger
Copy link
Member

The Hatch project uses a table, which I think looks very clean

https://github.com/pypa/hatch

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 4, 2023

I guess a table is OK, but I'd put it somewhere near the bottom of the document. I don't like the half-height empty first row though.

@hoechenberger
Copy link
Member

Bottom I fine with me too :)

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 4, 2023

With badges in a table:

https://github.com/mne-tools/mne-python/blob/6705df27b9ae840838c3ddc5022959f2782070de/README.rst

The small logo doesn't look good though I think.

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 4, 2023

BTW, now that badges are neatly compiled in a table, we can add more! I would at least add a badge that links to our docs.

@hoechenberger
Copy link
Member

@cbrnr Can you move the logo into the same line as the title? Is that possible?

I would also suggest to remove the information on installation via pip and instead only refer to our comprehensive install docs, esp since pip install mne only installs very limited functionality

@hoechenberger
Copy link
Member

hoechenberger commented Nov 4, 2023

BTW, now that badges are neatly compiled in a table, we can add more! I would at least add a badge that links to our docs.

Sounds great

Hahaha

I love how, without hesitation, we moved from, remove all those stupid badges! to let's add all the badges! just because we're now approaching their layout differently 😃

image

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 4, 2023

The small logo doesn't look good at all, so we're back with the old one:

https://github.com/mne-tools/mne-python/blob/3cc27c5efcbffee72666a059695f67857e8e8e7d/README.rst

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.

+1 for merge after fixing the duplicated word

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 4, 2023

Can you take a look at the to do at the top? I'd like to fix the underscore at least.

@hoechenberger
Copy link
Member

Now that you mentioned it, I cannot unsee it anymore!! What have you done :)

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 4, 2023

Yes! Help! I've also seen this in other places now. It is strange that the previous two rows look OK although they also contain two badges.

@hoechenberger
Copy link
Member

Is it a problem with that one specific badge perhaps?

@hoechenberger
Copy link
Member

Multi-row with less vertical whitespace 🫣 (I know you tried)

... which leaves us with multi-column ... I'd bite for multi-column at this time

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 6, 2023

The "openssf best practices in progress 96%" badge is way too long IMO. Can we shorten it to at least "openssf 96%"? There's no official badge for this, but we could create our own.

Speaking of openssf, the badge would probably also get smaller if we fulfilled the following missing requirements:

  1. "The project SHOULD respond to a majority (>50%) of enhancement requests in the last 2-12 months (inclusive)." – I guess we can set this to "yes"?
  2. "The project MUST have at least one primary developer who knows how to design secure software. (See ‘details’ for the exact requirements.)"
  3. "At least one of the project's primary developers MUST know of common kinds of errors that lead to vulnerabilities in this kind of software, as well as at least one method to counter or mitigate each of them."

For the last two points, I'd think that @drammock and/or @larsoner know about these things.

@hoechenberger
Copy link
Member

I dare say I also fulfill 2./3., so I think we have multiple devs to tick that one off ✅

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 6, 2023

I think I would also qualify, but depending on how official this is going to be, I don't really want to be the one on the list 😆! So we need someone willing to be that person.

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

I went through and updated a bunch of fields EDIT: on openssf. We are actually a few items away from gold I think.

@hoechenberger feel free to merge if you're happy

@larsoner
Copy link
Member

larsoner commented Nov 6, 2023

... and I personally don't mind the badge being too long. The table is still plenty small. I'd rather stick with their badge than roll our own custom one

@drammock
Copy link
Member

drammock commented Nov 6, 2023

Speaking of openssf, the badge would probably also get smaller if we fulfilled the following missing requirements:

@larsoner has marked these as "satisfied" (and #12175 gives us a way to track the first one)

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 6, 2023

Perfect! I think it looks pretty good now then! The rST underscore thing is still bugging me, so if anyone has an idea what the problem could be, please LMK.

@larsoner
Copy link
Member

larsoner commented Nov 6, 2023

I think the underscore is the best we can do at the moment for a reST substitution with a link unfortunately

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 6, 2023

I think the underscore is the best we can do at the moment for a reST substitution with a link unfortunately

So you are saying this is intentional behavior? I don't understand it though. Why can I put some badges side by side, whereas it obviously doesn't work with the Zenodo badge? (Note: I know almost nothing about reST!)

@larsoner
Copy link
Member

larsoner commented Nov 6, 2023

Not intentional in the sense that it's desired, I thought it was just a side effect of us using reST with a substitution + link. But looking deeper I think it's a GitHub reST rendering bug seeing as SciPy doesn't use substitutions and also has a gap between logos with a hyperlinked _:

https://github.com/scipy/scipy/blob/main/README.rst

Not sure about Zenodo

@hoechenberger
Copy link
Member

hoechenberger commented Nov 6, 2023

I think it's got something to do with the Zenodo SVG tbh

@hoechenberger
Copy link
Member

hoechenberger commented Nov 6, 2023

When you use an inspector and select the surrounding <a> element, it behaves differently for Zenodo than for all the other badges

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 6, 2023

Please stop me anytime, I'm just trying to understand this mysterious behavior. So do you think this is a bug in reST (i.e., how reST converts the syntax to HTML)? Or a bug in a specific reST engine (if that's even a thing)? Or a bug in how GitHub renders reST?

@drammock
Copy link
Member

drammock commented Nov 6, 2023

When you use an inspector and select the surrounding <a> element, it behaves differently for Zenodo than for all the other badges

if someone is motivated to dig, the Bandit repo README doesn't use the underscore-to-denote-link approach at all, they do:

.. image:: path/to/image.svg
   :target: https://url/of/target/

and they still have the underscore.

Or a bug in how GitHub renders reST?

That is the direction I'm leaning, but I haven't dug in enough to be confident.

@hoechenberger
Copy link
Member

Totally unrelated but it just crossed my mind, could you please check a11y implications of using a table without header? Is that acceptable at all?

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 6, 2023

Totally unrelated but it just crossed my mind, could you please check a11y implications of using a table without header? Is that acceptable at all?

Can you recommend a tool to do that?

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 6, 2023

I found this: https://www.powermapper.com/products/sortsite/rules/acchtmltablenoheaders/

I mean, it's already questionable if a table of badges are accessible in the first place.

@hoechenberger
Copy link
Member

So it seems common screen readers ship with heuristics to deal with such tables. I hence think it's acceptable for us to omit the header, even if not considered a good practice

@drammock
Copy link
Member

drammock commented Nov 6, 2023

from @cbrnr's link:

If a data table has headers marked up using td, then change these to th.
If the table is only used for layout add role=presentation to the table element.

I'm not sure how much control we actually have here, since the rST is getting converted to HTML by GitHub, not by us. In terms of a11y, probably the best we can do is to make sure each badge has alt text, and make sure the alt text is actually good.

As for getting rid of the underscore, just throwing out ideas here. I looked at the rendering of Bandit's README, and the difference is this:

bandit

So maybe try separating the badges with a non-breaking space? (u+00A0)

@cbrnr
Copy link
Contributor Author

cbrnr commented Nov 6, 2023

I already tried with nbsps, it didn't work (b49f014).

@hoechenberger
Copy link
Member

Shall we go ahead and merge here?

@larsoner
Copy link
Member

larsoner commented Nov 7, 2023

Yep looks like there are approvals, thanks @cbrnr !

@larsoner larsoner merged commit 7b6b795 into mne-tools:main Nov 7, 2023
larsoner added a commit to JD-Zhu/mne-python that referenced this pull request Nov 7, 2023
* upstream/main:
  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)
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)
@cbrnr cbrnr deleted the tweak-readme branch December 11, 2023 06:52
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
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