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

Migrate system notice to typescript #6666

Merged
merged 3 commits into from
Oct 20, 2020
Merged

Migrate system notice to typescript #6666

merged 3 commits into from
Oct 20, 2020

Conversation

vanya829
Copy link
Contributor

@vanya829 vanya829 commented Oct 6, 2020

Summary

Moved system notice component to Typescript.
Not all props we inline with their definitions, so I was forced to use any definition it a few places:

  • show method in notice type optional, because it missing in one of the notice lists in the tests
  • config param inside show method should be Partial<ClientConfig> based on selectors' definition, but it expects InstallationDate that is not a part of this type. That is why it's any
  • license param inside show method should be ClientLicense based on selectors' definition, but it expects IsLicensed that is not a part of this type. That is why it's any
  • dismissedNotices prop in SystemNotice component should be boolean based on selectors definition, but in component, it's used as an object with boolean props.

Ticket Link

Fixes mattermost/mattermost#13477

@mattermod
Copy link
Contributor

Hello @vanya829,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@hanzei hanzei added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester hacktoberfest-accepted labels Oct 7, 2020
hahmadia
hahmadia approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@hahmadia hahmadia left a comment

Choose a reason for hiding this comment

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

I looked and thought about the doubts you've raised and I honestly think it's okay to leave it as it is. Thank you for bringing them up and after thinking over them, I feel that what you've done is okay and there is no need for any changes. Thanks again!

gabrieljackson
gabrieljackson approved these changes Oct 8, 2020
View reviewed changes
Copy link
Contributor

@gabrieljackson gabrieljackson left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the contribution.

@gabrieljackson gabrieljackson removed the 2: Dev Review Requires review by a core commiter label Oct 8, 2020
@hanzei hanzei added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Oct 13, 2020
@hanzei
Copy link
Contributor

hanzei commented Oct 13, 2020

/update-branch

@hanzei hanzei requested a review from srkgupta October 13, 2020 07:40
@srkgupta
Copy link
Contributor

srkgupta commented Oct 13, 2020 •
edited
Loading

Hello @gabrieljackson or @hanzei
Can you let me know which screens are affected because of this change so that it can be tested accordingly.

@hanzei
Copy link
Contributor

hanzei commented Oct 14, 2020 •
edited
Loading

@srkgupta This PR affects the System Notice, which contains messages like https://github.com/vanya829/mattermost-webapp/blob/e8fd6355be72eba359829244c1d67df5e9bf0ffd/components/system_notice/notices.tsx#L93.

srkgupta
srkgupta approved these changes Oct 16, 2020
View reviewed changes
Copy link
Contributor

@srkgupta srkgupta left a comment

Choose a reason for hiding this comment

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

Thanks @vanya829 and @hanzei. Did some basic testing on the webapp and there were no issues/JS errors shown. Approving the PR.

@srkgupta srkgupta 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 Oct 16, 2020
@mm-cloud-bot
Copy link

Test server destroyed

@hahmadia
Copy link
Contributor

/update-branch

@hahmadia hahmadia added the AutoMerge used by Mattermod to merge PR automatically label Oct 20, 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.

@mattermod
Copy link
Contributor

Trying to auto merge this PR.

@mattermod mattermod merged commit 06cd05f into mattermost:master Oct 20, 2020
@mattermod
Copy link
Contributor

Pull Request successfully merged
SHA: 06cd05f