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

Migrate get link modal to be pure and use Redux #228

Merged
merged 3 commits into from
Nov 7, 2017

Conversation

tkbky
Copy link
Contributor

@tkbky tkbky commented Nov 1, 2017

Summary

Migrate get link modal to be pure and use Redux.

Ticket Link

mattermost-server/issues/7679

Checklist

  • Added or updated unit tests (required for all new features)

@tkbky
Copy link
Contributor Author

tkbky commented Nov 1, 2017

@saturninoabril Here is the PR. I'm stuck at getting the tests passed because of some unsupported DOM API by jsdom, namely queryCommandSupported and execCommand.

Related issues on jsdom:

Another finding is using the Object.defineProperty on document doesn't seem to work because of the Unforgeable properties in document that makes it not configurable as mentioned in this issue

@jasonblais jasonblais added 2: Dev Review Requires review by a core commiter Work in Progress Not yet ready for review and removed 2: Dev Review Requires review by a core commiter labels Nov 1, 2017
@jasonblais jasonblais requested a review from jwilander November 1, 2017 04:11
@saturninoabril
Copy link
Member

@tkbky I've found a workaround, on mocking like:

const supportedCommands = ['copy'];

Object.defineProperty(document, 'queryCommandSupported', {
  value: (cmd) => supportedCommands.includes(cmd)
});

Object.defineProperty(document, 'execCommand', {
  value: (cmd) => supportedCommands.includes(cmd)
});

Also added/modified some tests.

@saturninoabril saturninoabril added 2: Dev Review Requires review by a core commiter and removed Work in Progress Not yet ready for review labels Nov 6, 2017
@saturninoabril saturninoabril added this to the v4.5.0 milestone Nov 6, 2017
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@tkbky
Copy link
Contributor Author

tkbky commented Nov 7, 2017

@saturninoabril That looks great! Thanks for making the PR way better. 😄

@tkbky tkbky changed the title [WIP] Migrate get link modal to be pure and use Redux Migrate get link modal to be pure and use Redux Nov 7, 2017
@jwilander jwilander added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Nov 7, 2017
@jwilander
Copy link
Member

Awesome, thanks @tkbky

@jwilander jwilander merged commit 33f1da9 into mattermost:master Nov 7, 2017
@lindalumitchell lindalumitchell added the Tests/Not Needed Does not require new release tests label Nov 14, 2017
@jasonblais jasonblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Hacktoberfest Tests/Not Needed Does not require new release tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants