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

Mm 20561: Updating admin manage roles modal to typescript #4625

Merged
merged 4 commits into from Jan 21, 2020
Merged

Mm 20561: Updating admin manage roles modal to typescript #4625

merged 4 commits into from Jan 21, 2020

Conversation

opllama2
Copy link
Contributor

@opllama2 opllama2 commented Jan 7, 2020 •
edited by hanzei

Summary

This PR updates the admin console manage roles modal to use typescript within components/admin_console/manage_roles_modal

Ticket Link

Jira Link: https://mattermost.atlassian.net/browse/MM-20561
Fixes mattermost/mattermost#13511

@hanzei hanzei added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jan 7, 2020
@devinbinnie devinbinnie requested review from a team and removed request for devinbinnie January 7, 2020 16:14
@ghost ghost requested review from Adovenmuehle and wiggin77 and removed request for a team January 7, 2020 16:14
wiggin77
wiggin77 reviewed Jan 8, 2020
View reviewed changes
components/admin_console/manage_roles_modal/manage_roles_modal.tsx
@@ -14,8 +13,27 @@ import {trackEvent} from 'actions/diagnostics_actions.jsx';
import FormattedMarkdownMessage from 'components/formatted_markdown_message.jsx';
import BotBadge from 'components/widgets/badges/bot_badge';
import Avatar from 'components/widgets/users/avatar';
type Props = {
show: boolean;
user?: any;
Copy link
Member

Choose a reason for hiding this comment

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

Quick question: is the intention to make user a specific type when more code gets ported to TS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats a good question but i dont know about a plan to do so, imo its a good idea to do so if there's any "starter" type for the user object i can change to that type and we can append to it as we go.

wiggin77
wiggin77 approved these changes Jan 8, 2020
View reviewed changes
Copy link
Member

@wiggin77 wiggin77 left a comment

Choose a reason for hiding this comment

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

I'm not a subject matter expert regarding the webapp but this looks good to me. Later I would hope user and possibly error are made real types instead of any, but I understand that is hard to do until more code get ported.

@mattermod
Copy link
Contributor

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

@hanzei hanzei requested review from sudheerDev and removed request for Adovenmuehle January 19, 2020 11:24
@hanzei
Copy link
Contributor

hanzei commented Jan 20, 2020

@opllama2 Sorry for the delay with the reviews. Someone will look into your PR over the next few days.

@hanzei
Copy link
Contributor

hanzei commented Jan 20, 2020

/update-branch

sudheerDev
sudheerDev approved these changes Jan 20, 2020
View reviewed changes
Copy link
Contributor

@sudheerDev sudheerDev 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 @opllama2 🎉

@hanzei hanzei removed the 2: Dev Review Requires review by a core commiter label Jan 20, 2020
@hanzei hanzei added this to the v5.22.0 milestone Jan 20, 2020
saturninoabril
saturninoabril approved these changes Jan 21, 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.

Tested and passed, thanks @opllama2!

@saturninoabril saturninoabril added 4: Reviews Complete All reviewers have approved the pull request QA Review Done and removed 3: QA Review Requires review by a QA tester labels Jan 21, 2020
@saturninoabril saturninoabril merged commit c40a60e into mattermost:master Jan 21, 2020
@opllama2
Copy link
Contributor Author

Thnx all, proud of it my first contribution 🥰

@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sudheerDev sudheerDev sudheerDev approved these changes

@wiggin77 wiggin77 wiggin77 approved these changes

@saturninoabril saturninoabril saturninoabril 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 QA Review Done
Projects
None yet
Milestone
v5.22.0
7 participants
@opllama2 @mattermod @hanzei @sudheerDev @wiggin77 @saturninoabril @amyblais