Skip to content
This repository has been archived by the owner on Jul 30, 2019. It is now read-only.

Add focusin and focusout #1142

Merged
merged 6 commits into from
Feb 2, 2018
Merged

Add focusin and focusout #1142

merged 6 commits into from
Feb 2, 2018

Conversation

patricia-gallardo
Copy link
Contributor

This is my first draft for issue 278

@LJWatson
Copy link
Collaborator

This LGTM. Ping @chaals for final review.

@cynthia
Copy link
Member

cynthia commented Jan 17, 2018

Just as a passing remark - I think the commit message could have been worded/formatted better.

@cynthia
Copy link
Member

cynthia commented Jan 17, 2018

@chaals suggested I elaborate on how - so here is the list of options.

  1. Squash into a single commit and rewrite commit message. (this can be done with the "Squash and merge" button on GH's web UI.)
  2. Rebase. (This would be what would happen normally if there was critic available. Unfortunately that isn't the case, but it can be done from the CLI..)

Normally amend would be an option, but with the fixup commit that is a bit harder.

@chaals
Copy link
Collaborator

chaals commented Jan 17, 2018

Sangwhan suggested:

Squash into a single commit and rewrite commit message. (this can be done with the "Squash and merge" button on GH's web UI.)

Since I use the Web UI to Squash and merge, I rewrite the messages as relevant. (To the best of my ability...)

We should probably document this in our documentation, but I don't think you should worry about it overmuch for this case.

Copy link
Collaborator

@chaals chaals left a comment

Choose a reason for hiding this comment

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

The technical bit looks good...

Please

  • update the changes section to note this
  • add relevant acknowledgements
  • update the attributes index

@chaals chaals self-assigned this Jan 21, 2018
@siusin
Copy link
Contributor

siusin commented Jan 23, 2018

The test looks great! \o/

@patricia-gallardo could you update sections/events.include to add the focusin and focusout events as well?

@chaals chaals changed the title First draft for issue 278 Add focusin and focusout Jan 30, 2018
@chaals
Copy link
Collaborator

chaals commented Jan 30, 2018

Bruce will work on this on Friday (deadline day for the milestone)

@chaals chaals merged commit 9cf0d8c into master Feb 2, 2018
@patricia-gallardo
Copy link
Contributor Author

Thank you @brucelawson and @chaals for picking this up and finishing it. I still have quite a bit to learn.

@brucelawson brucelawson deleted the patricia-issue-278-focus-1 branch February 2, 2018 12:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants