Skip to content
This repository was archived by the owner on Oct 28, 2020. It is now read-only.

[Bug 882676] Missing indication in required form fields#464

Closed
caseybecking wants to merge 2 commits into
mozilla:masterfrom
caseybecking:master
Closed

[Bug 882676] Missing indication in required form fields#464
caseybecking wants to merge 2 commits into
mozilla:masterfrom
caseybecking:master

Conversation

@caseybecking
Copy link
Copy Markdown
Contributor

No description provided.

@glogiotatidis
Copy link
Copy Markdown
Contributor

Good job Casey, thanks for your first pull request!

Profile is looking great, can you please do the same for events as stated in the bug?

@caseybecking
Copy link
Copy Markdown
Contributor Author

Not perfect but look to be what you wanted to see. I think when we go back and fix the icons we should look at doing the required stars differently.

@glogiotatidis
Copy link
Copy Markdown
Contributor

Looks good. Agreed that we can mark as required differently / in a better way in a future re-design.

Two things left before we merge: You need to (1) merge the two commits into one, since both of them refer to the same bug (2) reword the commit message to read [fix bug XXXX] since you're fixing this bug ;)

Here are some steps to help you with (1) and (2). We'll be using git rebase a powerful command to do that kind of things:

  1. Use git rebase --interactive HEAD~2 to work on the last two commits of HEAD
  2. You should get an editor listing your last two commits and available commands for them
  3. fixup the second commit into the first
  4. reword the first commit
  5. Save and close editor.
  6. You should get a new editor, to edit the message of the first commit. Change to fix bug...
  7. Save and close editor
  8. The second commit should get merge without any further action from you
  9. Run git log, you should see one commit, with a new message. Check that everything is fine.
    1. Push to github using git push -f to force the push since you've now change git's history.
    2. Post a ping here so I can take a look.

That's it!

If you get stuck feel free to ping me on irc, in #remo-dev.

Thanks!

@glogiotatidis
Copy link
Copy Markdown
Contributor

@caseybecking any updates on this? If you need help to complete rebase steps don't hesitate to ping me on irc or reply here. Thanks!

@caseybecking
Copy link
Copy Markdown
Contributor Author

Sorry for the delay, I was able to complete the rebase. Let me know if i need to do anything else? Thank you.

@glogiotatidis
Copy link
Copy Markdown
Contributor

w00t your rebase went fine! One last important thing I didn't clarify: commit messages.

Commit messages must be on the present tense "Adds something" instead of "Added something" and should include the special text [fix bug XXXX] or [bug XXXX] along with a commit message.

This special text is captured by one of our bots and posts updates on bugzilla. If you use the fix bug tag instead of just bug it will also close the bug for you when we merge the commit! The rest of the commit message should say briefly that we change with this commit.

So the commit message for this pull request should be:

'[fix bug 882676] Add missing indication in required form fields.'

Can you can change this by rebasing and rewording.

Sorry for being too picky with the commit message, but it's our way to track things and credit people for their contributions.

Thanks!

@caseybecking
Copy link
Copy Markdown
Contributor Author

Still trying to do this one as the rebase doesnt seem to be pulling the same code now - should i be using something different than git rebase --interactive HEAD~2

@caseybecking
Copy link
Copy Markdown
Contributor Author

Lets try this one again. Let me know if that works.

@glogiotatidis
Copy link
Copy Markdown
Contributor

You should have used git rebase --interactive HEAD~1 because you only had one of your commits to rebase. Because you rebased two, you brought-in a commit from akatsoulas. Git magic, takes a while to get used to it ;)

No need to worry though: Your commit message and patch are perfect and I merged them in! You can see your commit here:

b71f72a

Nice job, thanks! 🍰 Let's go for the next one! :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants