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

Mm 20517 Migrate 'components/rhs_thread' module and associated tests to TypeScript #4907

Merged
merged 21 commits into from
May 15, 2020
Merged

Mm 20517 Migrate 'components/rhs_thread' module and associated tests to TypeScript #4907

merged 21 commits into from
May 15, 2020

Conversation

ikeohachidi
Copy link
Contributor

@ikeohachidi ikeohachidi commented Feb 14, 2020 •
edited by hanzei
Loading

Migrate 'components/rhs_thread' module and associated tests to TypeScript

Fixes mattermost/mattermost#13723
https://mattermost.atlassian.net/browse/MM-20517

@ikeohachidi ikeohachidi requested review from devinbinnie and saturninoabril and removed request for devinbinnie February 16, 2020 19:01
devinbinnie
devinbinnie suggested changes Feb 18, 2020
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 like a good start, just curious about the TODOs and some other comments.
Thanks :)

components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
@hanzei hanzei added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Feb 20, 2020
devinbinnie
devinbinnie approved these changes Feb 20, 2020
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.

Looking good now, thanks @ikeohachidi!

hmhealey
hmhealey suggested changes Feb 25, 2020
View reviewed changes
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I made quite a few suggestions for things that could be improved, but most of them are along the line of clarifying types (things like using Post instead of Record types) and clearing some stuff up with how redux actions are typed in components.

components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
components/rhs_thread/rhs_thread.tsx Outdated Show resolved Hide resolved
@saturninoabril
Copy link
Member

Will review once pending comments are resolved.

@hmhealey
Copy link
Member

hmhealey commented Mar 6, 2020

Hi @ikeohachidi. Is there anything we could help with on this?

@ikeohachidi
Copy link
Contributor Author

@hmhealey sorry i've just been a busy, but i'm now on it.

@saturninoabril
Copy link
Member

@ikeohachidi Please ping me once ready for review.

@saturninoabril saturninoabril removed the 3: QA Review Requires review by a QA tester label Apr 30, 2020
@ikeohachidi
Copy link
Contributor Author

@hmhealey Sorry i've taken so long to get back to this.
How do we get started with the battle on the type checker??

@hmhealey
Copy link
Member

hmhealey commented May 4, 2020

You'll have to convert FloatingTimestamp to TypeScript as well to be able to fix the typing issue. I think there's two other things you'll need to do with that:

  1. In FloatingTimestamp's index.ts, you can give it its OwnProps type to define the extra postId prop needed by its mapStateToProps.
  2. The type of this.state.topRhsPostId in the RhsThread component is incorrect. It should just be string instead of string | number

@ikeohachidi
Copy link
Contributor Author

Ok, on it

@ikeohachidi
Copy link
Contributor Author

@hmhealey please review

@ikeohachidi ikeohachidi requested a review from hmhealey May 10, 2020 20:50
@hmhealey
Copy link
Member

/update-branch

hmhealey
hmhealey approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Thanks for making that change! Apologies that it took so long to get back to reviewing this. It looks good to me now.

@hmhealey hmhealey added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core commiter labels May 14, 2020
@hmhealey
Copy link
Member

@saturninoabril This is ready for review now.

@saturninoabril saturninoabril added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label May 15, 2020
saturninoabril
saturninoabril approved these changes May 15, 2020
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.

Thanks @ikeohachidi, tested and passed!

@saturninoabril saturninoabril added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels May 15, 2020
@mm-cloud-bot
Copy link

Test server destroyed

@saturninoabril saturninoabril merged commit 7116e5a into mattermost:master May 15, 2020
@ikeohachidi ikeohachidi deleted the MM-20517 branch May 16, 2020 15:12
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels May 18, 2020
bradjcoughlin pushed a commit to bradjcoughlin/mattermost-webapp that referenced this pull request May 19, 2020
@ikeohachidi @mattermod @bradjcoughlin
Mm 20517 Migrate 'components/rhs_thread' module and associated tests …
74db5e3 
…to TypeScript (mattermost#4907)

* migrate to typescript

* migrate test to typescript

* add declaration file for react-custom-scrollbars

* update package-lock.json with new types

* remove comments

* update test

* fix issues

* fix refs api

* make some fixes

* update test

* add directTeammate

* fix type errors

* migrate floating_timestamp to typescript

* update types

* update snapshot

Co-authored-by: mattermod <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hmhealey hmhealey hmhealey 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
Projects
None yet
Milestone
No milestone
8 participants
@ikeohachidi @saturninoabril @hmhealey @mm-cloud-bot @devinbinnie @hanzei @amyblais @mattermod