Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

[MM-22937] Fix Edge case by using includes instead of contains #5077

Merged
merged 3 commits into from
Mar 24, 2020
Merged

[MM-22937] Fix Edge case by using includes instead of contains #5077

merged 3 commits into from
Mar 24, 2020

Conversation

fmunshi
Copy link
Contributor

@fmunshi fmunshi commented Mar 18, 2020

Summary

  • Fixes an issue where dragging and dropping a file on pre-chromium edge would result in console errors and the overlay not appearing
  • Edge does not support .contains so replacing it with .includes fixes the drag and drop issue
  • This issue is only reproducible on pre-chromium edge.

Ticket Link

https://mattermost.atlassian.net/browse/MM-22937

@fmunshi fmunshi added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Mar 18, 2020
@fmunshi fmunshi requested a review from ogi-m March 18, 2020 22:39
@amyblais amyblais added this to the v5.22.0 milestone Mar 18, 2020
@amyblais amyblais added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Mar 18, 2020
Willyfrog
Willyfrog approved these changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@Willyfrog Willyfrog left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@hmhealey hmhealey removed the 2: Dev Review Requires review by a core commiter label Mar 19, 2020
@ogi-m ogi-m added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Mar 23, 2020
ogi-m
ogi-m approved these changes Mar 23, 2020
View reviewed changes
Copy link

@ogi-m ogi-m left a comment

Choose a reason for hiding this comment

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

Tested and looks good

  • overlay show in Edge
  • no console error
  • working as expected in Chrome & Desktop app

@ogi-m ogi-m added QA Review Done and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Mar 23, 2020
@mattermod
Copy link
Contributor

Test server destroyed

@fmunshi fmunshi added the 4: Reviews Complete All reviewers have approved the pull request label Mar 23, 2020
@fmunshi fmunshi force-pushed the MM-22937-use-includes-instead-of-contains branch from eb248c8 to bd189ad Compare March 24, 2020 00:03
@Willyfrog
Copy link
Contributor

@fm2munsh can you fix the tests before we can merge?

@fmunshi
Copy link
Contributor Author

fmunshi commented Mar 24, 2020 •
edited
Loading

@Willyfrog Its the build step failing with some mattermost-redux issue (also seems to be happening on master - 8729c9b) not sure whats causing it... but its definitely not related to this PR

@fmunshi
Copy link
Contributor Author

fmunshi commented Mar 24, 2020

Looks like the issue is highlighted here https://mattermost.atlassian.net/browse/MM-23538

@fmunshi fmunshi added the AutoMerge used by Mattermod to merge PR automatically label Mar 24, 2020
@mattermod
Copy link
Contributor

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

@fmunshi fmunshi merged commit 59aa12c into mattermost:master Mar 24, 2020
@fmunshi fmunshi deleted the MM-22937-use-includes-instead-of-contains branch March 24, 2020 19:11
@mattermost-build mattermost-build mentioned this pull request Mar 24, 2020
Merged
@mattermod mattermod added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Mar 24, 2020
@mattermod mattermod removed the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Mar 24, 2020
@mattermod
Copy link
Contributor

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

@fmunshi fmunshi removed the AutoMerge used by Mattermod to merge PR automatically label Mar 24, 2020
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Changelog/Done Required changelog entry has been written and removed Changelog/Not Needed Does not require a changelog entry labels Mar 25, 2020
@ogi-m ogi-m added the Tests/Not Needed Does not require new release tests label Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Willyfrog Willyfrog Willyfrog approved these changes

@hmhealey hmhealey hmhealey approved these changes

@ogi-m ogi-m ogi-m approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation QA Review Done Tests/Not Needed Does not require new release tests
Projects
None yet
Milestone
v5.22.0
6 participants
@fmunshi @mattermod @Willyfrog @hmhealey @ogi-m @amyblais