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

MM-20078: Migrate 'components/admin_console/radio_setting.jsx' and associated tests to TypeScript #4202

Merged
merged 9 commits into from
Nov 15, 2019

Conversation

allenl7
Copy link
Contributor

@allenl7 allenl7 commented Nov 13, 2019

Summary

Migrate 'components/admin_console/radio_setting.jsx' and associated tests to TypeScript

Ticket Link

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

@allenl7 allenl7 marked this pull request as ready for review November 13, 2019 04:20
@jespino jespino added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Nov 13, 2019
Copy link
Contributor

@lindy65 lindy65 left a comment

Choose a reason for hiding this comment

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

Thanks @allenlai18 👍

Would you be open to adding tests for your PR please?

@lindy65 lindy65 added QA Review Done Tests/Not Needed Does not require new release tests and removed 3: QA Review Requires review by a QA tester labels Nov 13, 2019
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.

LGTM! Thanks @allenlai18!

@allenl7
Copy link
Contributor Author

allenl7 commented Nov 13, 2019

Thanks @allenlai18 👍

Would you be open to adding tests for your PR please?

@lindy65 Yes. I would be open. What test are you talking about though?

@lindy65
Copy link
Contributor

lindy65 commented Nov 14, 2019

Hi @allenlai18 :)

Thanks, In the title it mentions 'associated tests' so I'm not sure whether any unit tests existed already? If they do, then it would be converting them to TypeScript. If none exist, it would mean writing unit tests for this PR...

@jespino
Copy link
Member

jespino commented Nov 14, 2019

@allenlai18 you can base your tests in other similar files in the same directory, for example color_setting.test.tsx.

@allenl7
Copy link
Contributor Author

allenl7 commented Nov 14, 2019

Hi @allenlai18 :)

Thanks, In the title it mentions 'associated tests' so I'm not sure whether any unit tests existed already? If they do, then it would be converting them to TypeScript. If none exist, it would mean writing unit tests for this PR...

@lindy65 @jespino :) Gotcha, so there is exists a unit test for radio_setting.tsx but it's for the one in /widgets/settings/ . I migrated the one in admin_console. They both have the exact same component name and filename. Would it be better to rename them?

I will write unit test for admin_console RadioSetting

@lindy65
Copy link
Contributor

lindy65 commented Nov 14, 2019

Thanks @allenlai18, appreciated 👍

I'll defer to @jespino for your question on renaming...

@jespino
Copy link
Member

jespino commented Nov 14, 2019

I don't think so, one of the is a widget that is design to be generally used (but is only used in one place for now) and the other is scoped to the admin console, I think we can keep both as they are.

Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

Now looks good to me. Thanks @allenlai18! 🎉

@allenl7
Copy link
Contributor Author

allenl7 commented Nov 14, 2019

Now looks good to me. Thanks @allenlai18! 🎉

@jespino I do not have much experience in writing unit test. please let me know if there are any suggestions or things that I could have done better!

--
it looks like its failling expect(received).toMatchSnapshot(). do i need to create a snapshot and push that?

@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 Nov 14, 2019
@jespino
Copy link
Member

jespino commented Nov 15, 2019

@allenlai18 yes, you had to add it, thanks

@jespino jespino merged commit 82673f1 into mattermost:master Nov 15, 2019
@jespino
Copy link
Member

jespino commented Nov 15, 2019

Merged! thanks @allenlai18! 🎉

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 QA Review Done Tests/Not Needed Does not require new release tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants