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

Style updates to find an issue page #15872

Merged
merged 1 commit into from Aug 28, 2015

Conversation

lyndaoleary
Copy link
Contributor

  • Fixed exp/beginner label colour background
  • Added thead element to label table
  • Updated issue list screenshot (old one referenced old whitebelt label)
  • Content tweaks as per style guide

Signed-off-by: Lynda O'Leary lyndaoleary29@gmail.com

@lyndaoleary
Copy link
Contributor Author

Hello, I am a new contributor. I would like to help out on any doc issues. While familiarizing myself with the style guide and contribution guidelines, I spotted a few style issues.

I also noticed that the gh-label beginner style wasn't being applied to the labels correctly, it looks to be the way the styles and comments are being parsed. I have fixed this in this PR.

Let me know if there's anything I need to change, thanks for your time and patience on this! 😄

@SvenDowideit
Copy link
Contributor

LGTM :) @moxiegirl @thaJeztah

@@ -107,9 +103,9 @@ In this section, you find and claim an open documentation lines issue.

![Open issues](/project/images/issue_list.png)

3. Look for the <strong class="gh-label beginner">exp/beginner</strong> items on the list.
3. Look for the <strong class="gh-label beginner">exp/beginner</strong> issues.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this step could be removed completely? Once the exp/beginner label is applied in step 4 the user will only see the relevant issues anyway. Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tombee Good point. Maybe the thing to do is #4 and then have them look in the sorted list for "kind/writing" or some other kind. @lyndaoleary your choice.

@moxiegirl
Copy link
Contributor

@lyndaoleary Nice and good to see ya on the project. Maybe wait for the @thaJeztah to comment. After that any changes, rebase/squash, and we can merge. Thank you.

@thaJeztah
Copy link
Member

First of all, great to see you started contributing @lyndaoleary, thanks so much for that!

The changes look really good. The only comment I have is that your new screenshot is a retina screenshot, and quite large because of that. I've prepared two optimized versions of your screenshot (one losslessly optimized to reduce the file size, and one resized version), and uploaded them to WeTransfer; http://we.tl/3vVoOXXW7k, could you use one of those to prevent the repository from "ballooning"?

@thaJeztah
Copy link
Member

@moxiegirl While reading this, I realized that this documentation is no longer accurate; some time ago, the issue triaging process was updated; #14719, and new labels were decided on.

However, the labels were never adjusted to the new proces after that was merged #14719 (comment)

So, I think a number of things have to be done to fix this;

  • New labels have to be added on github and deprecated labels removed
  • Existing issues have to be relabeled where needed (I think this has been done before, but I don't know if there's an easy way to do this, or to automate this)
  • This docs has to be adjusted to match the new situation

This is beyond the scope of this PR, but I wanted to bring it up 😄

I'm good with merging this PR now (after the optimized image was added), because it's a good improvement 👍

* Fixed exp/beginner label colour background
* Added thead element to label table
* Updated issue list screenshot (old one referenced old whitebelt label)
* Content tweaks as per style guide

Signed-off-by: Lynda O'Leary <lyndaoleary29@gmail.com>
@lyndaoleary
Copy link
Contributor Author

@moxiegirl @SvenDowideit @thaJeztah
Thanks for all the feedback. I changed the following:

  • Added resized and optimized screenshot (Thanks 😄 @thaJeztah)
  • Removed step 3 as discussed & updated numbering
  • Reordered steps 5 and 6
  • Replaced we with The project...

@thaJeztah I read through your comments and would like to work on updating this document as per the new issue triage process 👍


7. When you find an open issue that both interests you and is unclaimed, add a
`#dibs` comment.
7. Your issue # will be different depending on what you claimed. After a moment, Gordon the Docker
Copy link
Member

Choose a reason for hiding this comment

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

Oh, looks like you skipped 6

Since you want to contribute more;
Sometimes reviewers leave a comment like s/foo/bar/. You may not be familiar with that; this is shorthand for "replace 'foo' with 'bar'". It's basically how one would replace a text with sed, and it's to save time reviewing.

@thaJeztah
Copy link
Member

Great work! I added one "nit", but LGTM otherwise.

Thanks for offering to help after the labels are adjusted! I'll check with @moxiegirl who/when we will do the label change/migration and will let you know when that's done.

I think that with the rewrite, we should also explain finding an issue by "kind"; not all issues have an "experience" label added, and people may want to look for issues in a specific "area" to work on; browsing through issues that are "popular" (most commented on), could be a clear indicator that it's something to work on. (That's just me thinking out loud)

@moxiegirl
Copy link
Contributor

LGTM for me. Thanks again @lyndaoleary @thaJeztah if you can forgo your nit given Lynda is gonna come back and update...let's merge.

@thaJeztah
Copy link
Member

it's just a nit, so let's merge!

thanks @lyndaoleary

thaJeztah added a commit that referenced this pull request Aug 28, 2015
@thaJeztah thaJeztah merged commit 279bbbe into moby:master Aug 28, 2015
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.

None yet

6 participants