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

fix(modal): give a reason of rejection when escape key pressed #1991

Closed
wants to merge 1 commit into from
Closed

fix(modal): give a reason of rejection when escape key pressed #1991

wants to merge 1 commit into from

Conversation

tkrotoff
Copy link
Contributor

Make it symmetric with $modalStack.dismiss(modal.key, 'backdrop click')

Fix for #1956

Make it symmetric with $modalStack.dismiss(modal.key, 'backdrop click')
@chrisirhc
Copy link
Contributor

Looks good to me. Although "press" may be more consistent, instead of the past tense.

@tkrotoff
Copy link
Contributor Author

I would have changed both to past tense:

  • 'backdrop clicked'
  • 'escape key pressed'

Instead of:

  • 'backdrop click'
  • 'escape key press'

(sounds better to me, although english is not my mother tongue).

@chrisirhc
Copy link
Contributor

It does sound better logically but I think that putting past tenses in code makes it less accessible to users that aren't very familiar with the English language. The fact that they're also very similar (just two extra letters) makes it easy for users to make mistake.

Since DOM events are always in present tense, I think it's a better idea to keep it similar to DOM events, and use present tense.

With that in mind, it might make sense to keep this consistent with the DOM event that triggered it, so something like: escape keypress or escape keydown.

@bekos bekos closed this in cb31b87 Apr 12, 2014
@bekos
Copy link
Contributor

bekos commented Apr 12, 2014

@tkrotoff Made the change suggested by @chrisirhc and landed. Thx :-)

@tkrotoff tkrotoff deleted the escape_dismiss_reason branch February 20, 2015 15:43
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