-
Notifications
You must be signed in to change notification settings - Fork 550
Conversation
This LGTM. Ping @chaals for final review. |
Just as a passing remark - I think the commit message could have been worded/formatted better. |
@chaals suggested I elaborate on how - so here is the list of options.
Normally amend would be an option, but with the fixup commit that is a bit harder. |
Sangwhan suggested:
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. |
There was a problem hiding this 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
The test looks great! \o/ @patricia-gallardo could you update sections/events.include to add the focusin and focusout events as well? |
Bruce will work on this on Friday (deadline day for the milestone) |
Thank you @brucelawson and @chaals for picking this up and finishing it. I still have quite a bit to learn. |
This is my first draft for issue 278