This repository has been archived by the owner on Mar 13, 2024. It is now read-only.
[MM-17406] Check if event target for setting item max is focused inside setting list before saving #3457
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…de setting list before saving
devinbinnie
added
2: Dev Review
ghost
requested review from
devinbinnie
requested review from
sudheerDev and
a team
and removed request for
a team
|
Changes looks good. This will be tested as part of 5.15 release. Removing the QA Review label. |
asaadmahmood
approved these changes
Aug 17, 2019
sudheerDev
reviewed
Aug 21, 2019
| @@ -134,7 +134,7 @@ export default class SettingItemMax extends React.PureComponent { | |||
| if (this.props.shiftEnter && e.keyCode === Constants.KeyCodes.ENTER && e.shiftKey) { | |||
| return; | |||
| } | |||
| if (isKeyPressed(e, Constants.KeyCodes.ENTER) && this.props.submit && e.target.tagName !== 'SELECT' && e.target.parentElement && e.target.parentElement.className !== 'react-select__input' && !e.target.classList.contains('btn-cancel')) { | |||
| if (isKeyPressed(e, Constants.KeyCodes.ENTER) && this.props.submit && e.target.tagName !== 'SELECT' && e.target.parentElement && e.target.parentElement.className !== 'react-select__input' && !e.target.classList.contains('btn-cancel') && this.settingList.current && this.settingList.current.contains(e.target)) { | |||
There was a problem hiding this comment.
If we happen to add anymore to this condition consider splitting them into different consts by grouping these conditions.
sudheerDev
approved these changes
Aug 21, 2019
devinbinnie
changed the title
|
@stylianosrigas Please prioritize reviewing this. |
mattermod
added
AutomatedCherryPick
and removed
CherryPick/Approved
devinbinnie
pushed a commit
that referenced
this pull request
Aug 29, 2019
amyblais
added
Changelog/Done
skheria
pushed a commit
to uber-archive/mattermost-webapp
that referenced
this pull request
Oct 3, 2019
…de setting list before saving (mattermost#3457) * [MM-17406] Check if event target for setting item max is focused inside setting list before saving * Fixed a style issue
skheria
pushed a commit
to uber-archive/mattermost-webapp
that referenced
this pull request
Oct 3, 2019
…de setting list before saving (mattermost#3457) * [MM-17406] Check if event target for setting item max is focused inside setting list before saving * Fixed a style issue
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Existing behaviour was allowing any ENTER press on the DOM while a setting item was open to trigger the save on said setting list. This PR ensures that the focused element must be within the setting list on save.
Ticket Link
https://mattermost.atlassian.net/browse/MM-17406