Skip to content

Conversation

@drammock
Copy link
Member

tries to make the contrib guide more approachable.

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.

A much clearer and more inclusive introduction, +1 for merge from me

@drammock
Copy link
Member Author

Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

Great initiative! I've added one suggestion

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

Thx @drammock

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
@hoechenberger
Copy link
Member

hoechenberger commented Nov 22, 2021

I wonder if we can find a better icon to symbolize bug fixes? The current choice is so much larger than the rest and not monochrome 🤔

@britta-wstnr
Copy link
Member

Looks great, @drammock !
While we are at it, would it be okay to nitpick on two small things in the existing text ... ?

@larsoner
Copy link
Member

While we are at it, would it be okay to nitpick on two small things in the existing text ... ?

I would say yes, even better if you just want to push a commit to @drammock's branch to fix them!

@drammock
Copy link
Member Author

I wonder if we can find a better icon to symbolize bug fixes? The current choice is so much larger than the rest and not monochrome

We could make it monochrome quite easily. The size might be tricky but should be possible. I'm also open to using Unicode emoji instead of fontawesome, if there are better choices there

While we are at it, would it be okay to nitpick on two small things in the existing text?

Yes go right ahead

@drammock
Copy link
Member Author

@britta-wstnr
Copy link
Member

I can add the changes tomorrow, did not get to it today. Just quickly from my notes, in case someone wants to beat me to it:
I suggest to change line 60

then `pushing`_ the local changes up to your fork),

to be more explicit:

then `pushing`_ the local changes up to your fork on GitHub),
  • I think this is something that is hard to grasp at the beginning, where which version of the code lives.

And then, under Running the test suite, the sidebar (pytest flags) creates a lot of awkward white space as it is longer than the text to its left (and constrained by the code bars). Maybe it could be placed at the end of the paragraph using the whole width?

But as I said - very much nitpicking :)

@hoechenberger
Copy link
Member

* make line about fork clearer

* minimize white space and improve wording

* improve some wording
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.

Thanks, feel free to merge and backport @drammock

@drammock drammock merged commit 955f914 into mne-tools:main Nov 23, 2021
drammock added a commit that referenced this pull request Nov 23, 2021
* make contrib guide more friendly

* move icons to beginning

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

* more consistent icon style

* Additions to contrib guide PR (#6)

* make line about fork clearer

* minimize white space and improve wording

* improve some wording

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Co-authored-by: Britta Westner <britta.wstnr@gmail.com>
@drammock drammock deleted the update-contrib-guide branch November 23, 2021 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants