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

[MM 23264] Show Confirmation Modal for group mentions #5065

Conversation

hahmadia
Copy link
Contributor

Summary

Show the confirmation modal for group mentions

Ticket Link

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

Related Pull Requests

mattermost/mattermost#14068
mattermost/mattermost-redux#1086

@hahmadia hahmadia added the Work in Progress Not yet ready for review label Mar 17, 2020
@amyblais amyblais added this to the v5.22.0 milestone Mar 17, 2020
@fmunshi fmunshi self-requested a review March 17, 2020 15:39
@hahmadia hahmadia added 1: UX Review Requires review by a UX Designer 2: Dev Review Requires review by a core commiter labels Mar 17, 2020
Copy link
Contributor

@michaelgamble michaelgamble left a comment

Choose a reason for hiding this comment

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

Looked over the screens with Hossein, everything looks good to go on this one from my perspective.

@hahmadia hahmadia removed the 1: UX Review Requires review by a UX Designer label Mar 17, 2020
@hahmadia hahmadia removed the Work in Progress Not yet ready for review label Mar 17, 2020
@hahmadia hahmadia force-pushed the MM-23264-ShowConfirmationModalForGroupMention branch from c9a4dfd to 37deb4c Compare March 17, 2020 23:55
Copy link
Contributor

@fmunshi fmunshi left a comment

Choose a reason for hiding this comment

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

👌

values={Object {}}
/>
}
title=""
Copy link
Contributor

@catalintomai catalintomai Mar 18, 2020

Choose a reason for hiding this comment

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

why are these emptied (throughout the file)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because prior to my change, the title were always getting set regardless if the confirmation modal was being shown or not. In my change, I start them off as blank and only set them if needed. So they don't won't always have values associated to them unless we will display the confirmation modal.

Copy link
Contributor

@catalintomai catalintomai left a comment

Choose a reason for hiding this comment

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

Left some suggestions/questions but looks great otherwise.

@hahmadia hahmadia force-pushed the MM-23264-ShowConfirmationModalForGroupMention branch from 5099a43 to 3a5a6dd Compare March 18, 2020 01:36
make warning dialogue appear for group mentions and all channel mentions

fix linting and i18n extract

address UX concerns regarding wording for group dialogue

fix lint and add tests

address PR comments

update text in UI to include group mentions
@hahmadia hahmadia force-pushed the MM-23264-ShowConfirmationModalForGroupMention branch from 63fafb5 to d4f69ab Compare March 18, 2020 01:42
@hahmadia hahmadia added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Mar 18, 2020
@hahmadia hahmadia added the Do Not Merge Should not be merged until this label is removed label Mar 18, 2020
@amyblais amyblais removed this from the v5.22.0 milestone Mar 18, 2020
@srkgupta srkgupta self-requested a review April 7, 2020 19:13
@srkgupta srkgupta added 3: QA Review Requires review by a QA tester and removed 4: Reviews Complete All reviewers have approved the pull request labels Apr 7, 2020
@srkgupta srkgupta added this to the v5.24.0 milestone Apr 9, 2020
@hahmadia hahmadia closed this Apr 14, 2020
@hahmadia hahmadia deleted the MM-23264-ShowConfirmationModalForGroupMention branch April 14, 2020 22:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3: QA Review Requires review by a QA tester Do Not Merge Should not be merged until this label is removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants