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

Migrate position utils to typescript #3813

Merged
merged 1 commit into from
Oct 10, 2019
Merged

Migrate position utils to typescript #3813

merged 1 commit into from
Oct 10, 2019

Conversation

guigui64
Copy link
Contributor

Summary

Migrate 'utils/position_utils.jsx' and associated test to TypeScript

Ticket Link

Fixes mattermost/mattermost#12487

@devinbinnie devinbinnie added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Oct 1, 2019
devinbinnie
devinbinnie suggested changes Oct 1, 2019
View reviewed changes
Copy link
Member

@devinbinnie devinbinnie 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! Just a couple small comments.

utils/position_utils.tsx Outdated Show resolved Hide resolved
utils/position_utils.tsx Outdated Show resolved Hide resolved
saturninoabril
saturninoabril approved these changes Oct 2, 2019
View reviewed changes
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.

LGTM, thanks @guigui64!

@saturninoabril saturninoabril added QA Review Done and removed 3: QA Review Requires review by a QA tester labels Oct 2, 2019
@sudheerDev
Copy link
Contributor

@guigui64 If you pick other tickets can you rename the files into tsx? I am not sure if you made new files and copied the contents because the diff here shows them as new files. If we rename we can keep the git history intact so people can check when and why a certain change is made.

@sudheerDev
Copy link
Contributor

@guigui64 Thanks for the contribution. Looks good to me.

devinbinnie
devinbinnie approved these changes Oct 5, 2019
View reviewed changes
Copy link
Member

@devinbinnie devinbinnie 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 now, thanks @guigui64 !

@devinbinnie devinbinnie added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Oct 5, 2019
@devinbinnie
Copy link
Member

@guigui64 Can you sync your PR with master? Once that's done I can go ahead and merge it.

@devinbinnie devinbinnie merged commit 8e20953 into mattermost:master Oct 10, 2019
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Oct 10, 2019
Wicked7000 pushed a commit to Wicked7000/mattermost-webapp that referenced this pull request Oct 12, 2019
@guigui64 @Wicked7000
Migrate position utils to typescript (mattermost#3813)
8b43ec8
brewsterbhg pushed a commit to brewsterbhg/mattermost-webapp that referenced this pull request Nov 11, 2019
@guigui64 @brewsterbhg
Migrate position utils to typescript (mattermost#3813)
4badb7e
@lindy65 lindy65 added the Tests/Not Needed Does not require new release tests label Nov 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sudheerDev sudheerDev sudheerDev approved these changes

@saturninoabril saturninoabril saturninoabril approved these changes

@devinbinnie devinbinnie devinbinnie approved these changes

Assignees
No one assigned
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 QA Review Done Tests/Not Needed Does not require new release tests
Projects
None yet
Milestone
No milestone
6 participants
@guigui64 @sudheerDev @devinbinnie @saturninoabril @lindy65 @amyblais