search

mozilla / addon-recommendation-shield-study

Stand-alone verison of Add-on Recommendation for Shield Study
Mozilla Public License 2.0
3 stars 7 forks source link

Closes mozilla/addon-recommendation-shield-study#12 #19

Closed casebenton closed 8 years ago

casebenton commented 8 years ago

r?

Osmose commented 8 years ago

Your commit message should explain what the commit changes (see http://chris.beams.io/posts/git-commit/ for some generally good guidelines for writing commit messages, although none of those are hard rules and we break some of them ourselves). You can still have the auto-issue-closing stuff in there, but that can just be the very last line in the commit:

Hide recommendation panel whenever a tab is deactivated.

Fixes #12
casebenton commented 8 years ago

Ok, I'll start leaving more detailed messages. How does the commit message in https://github.com/mozilla/addon-recommendation-shield-study/pull/21 look?

Osmose commented 8 years ago

Ok, I'll start leaving more detailed messages. How does the commit message in #21 look?

It's better, but it doesn't explain what the commit is doing, it just says that flexbox stuff is changing. Fix padding issues in recommendation panel would be better. Consider that commit messages will be read by:

  1. People reviewing your code who don't yet know what changes you made.
  2. People in the future (possibly yourself) who are reading back to see when certain changes happened and trying to figure out why they happened.

To either of those people, just nothing that some changes happened without mentioning why or what isn't useful.

Osmose commented 8 years ago

Tested the fix locally and it seems to work fine, I couldn't find any obvious edge cases. Fix the commit message and I'll r+ this and merge it. Nice work!

casebenton commented 8 years ago

Ok, I made the message more detailed. Thanks!