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

MM-10541 Update announcement banner immediately without refresh #1199

Merged
merged 3 commits into from
May 11, 2018

Conversation

jwilander
Copy link
Member

@jwilander jwilander commented May 9, 2018

Summary

Just needed to update state again when one of the banner settings in the props changed.

The behavior is as follows:

If the banner is being enabled for the first time:

  • Right after enabling and saving the banner settings, the banner will be immediately shown to users with the settings as set

If the banner has not been dismissed by the user:

  • Changes to the enabled, text, text color, banner color or allow dismissal settings will be reflected immediately in all users' clients

If the banner has been dismissed by the user:

  • Changes to text or allow dismissal settings will cause the banner to show again
  • Changes to enabled, text color or banner color will not cause the banner to show again (enabled because if you're disabling it wouldn't show the banner either way)

Ticket Link

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

@jwilander jwilander added the 2: Dev Review Requires review by a core commiter label May 9, 2018
@jwilander jwilander added this to the v4.10.0 milestone May 9, 2018
Copy link
Member

@lieut-data lieut-data left a comment

Choose a reason for hiding this comment

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

Looks good to me: is this something we can unit test?

@jwilander
Copy link
Member Author

Good call, I'll add some tests

nextProps.bannerColor !== this.props.bannerColor ||
nextProps.bannerTextColor !== this.props.bannerTextColor ||
nextProps.allowBannerDismissal !== this.props.allowBannerDismissal) {
this.setState(this.getState(nextProps));
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a good place where we could try out https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops :)

@hmhealey hmhealey added Awaiting Submitter Action Blocked on the author and removed 2: Dev Review Requires review by a core commiter labels May 9, 2018
@jwilander jwilander added 4: Reviews Complete All reviewers have approved the pull request and removed Awaiting Submitter Action Blocked on the author labels May 9, 2018
@GoldUniform GoldUniform merged commit 51cdff4 into release-4.10 May 11, 2018
@GoldUniform GoldUniform deleted the mm-10541 branch May 11, 2018 16:59
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels May 11, 2018
@lindalumitchell lindalumitchell added the Tests/Not Needed Does not require new release tests label May 22, 2018
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 Tests/Not Needed Does not require new release tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants