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

[MM-12068] Add ability to remove custom branding image #3207

Merged
merged 21 commits into from Aug 27, 2019

Conversation

hahmadia
Copy link
Contributor

@hahmadia hahmadia commented Jul 23, 2019

NOTE
I am not very experienced and understanding of how the styling works within the APP. If someone could explain how to style a className that I attach to a <div/>, I would greatly appreciate it because I couldn't figure it out. Other than the styling, I think all else should be okay (unless I have implemented something incorrectly)

As a junior dev and a first contribution, all and any advice would be greatly appreciated.
Thank you!

Summary

This ticket gives the user the ability to remove custom branding image

Ticket Link

Fixes mattermost/mattermost#9507

Demonstration Video

MM-12068

@hanzei
Copy link
Contributor

hanzei commented Jul 23, 2019

Hey @hahmadia,

looks like you have a linter error:

eslint --ignore-pattern node_modules --ignore-pattern non_npm_dependencies --ignore-pattern dist --ext .js --ext .jsx . --quiet


/root/mattermost-webapp/components/admin_console/schema_admin_settings.jsx
  600:41  error  use `Boolean(saveNeeded)` instead  no-implicit-coercion

@hahmadia
Copy link
Contributor Author

Thanks for pointing that out @hanzei. I have fixed all lint + translation issues.

@hanzei hanzei added 1: PM Review Requires review by a product manager Setup Cloud Test Server Setup a test server using Mattermost Cloud and removed Work in Progress Not yet ready for review Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Jul 23, 2019
@hanzei hanzei requested a review from esethna July 23, 2019 20:46
@hanzei hanzei added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Jul 23, 2019
@hahmadia hahmadia force-pushed the MM-12068 branch 2 times, most recently from 6aa77e0 to 355c6e6 Compare July 28, 2019 02:42
@esethna esethna requested review from wiersgallak and removed request for esethna July 29, 2019 15:47
@esethna
Copy link
Contributor

esethna commented Jul 29, 2019

Thanks for the PR @hahmadia! @asaadmahmood may be able to help provide some styling guidance

@asaadmahmood
Copy link
Contributor

One thing that's not working is if I lets say I don't have any image uploaded, but if I do upload it, the save button does not turn blue, I have to change some other setting to make it recognise that something has changed.

@asaadmahmood
Copy link
Contributor

@hahmadia I have updated the css, please pull my changes from your remote branch before making any changes locally and pushing them.

@asaadmahmood
Copy link
Contributor

@esethna We have an outstanding question on UX, which is, whether the save button should detect a change when a user uploads a picture.

The way it's currently implemented is that if a user uploads a picture, the save button does not change because I think it automatically uploads and saves it without the user performing a "Save" action like he does for all the other fields.

@esethna
Copy link
Contributor

esethna commented Jul 30, 2019

Thanks @hahmadia @asaadmahmood, as per the designs the user should have to hit the "Save" button after selecting an image from the file window:

image

https://projects.invisionapp.com/share/T6O2PN04EFC#/screens/320132211

@mattermost mattermost deleted a comment from mattermod Jul 31, 2019
@mattermost mattermost deleted a comment from mattermod Jul 31, 2019
@mattermost mattermost deleted a comment from mattermod Jul 31, 2019
@mattermost mattermost deleted a comment from mattermod Jul 31, 2019
@mattermost mattermost deleted a comment from mattermod Jul 31, 2019
@mattermost mattermost deleted a comment from mattermod Jul 31, 2019
@mattermost mattermost deleted a comment from mattermod Jul 31, 2019
@mattermost mattermost deleted a comment from mattermod Jul 31, 2019
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.

Functionality looks good. Only few requests, thanks @hahmadia!

error({id: err.server_error_id, ...err});
}
}

Copy link
Member

Choose a reason for hiding this comment

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

We're actually moving away from this pattern into a more redux-friendly format which is dispatching an action directly. That can be done by creating components/admin_console/brand_image_setting/index.js and from there pass uploadBrandImage and deleteBrandImage as actions into brand_image_setting.jsx. That would be not necessary for this PR but good to have.
Let me know what's your plan. That could also be done on separate PR if you're interested.

Copy link
Member

Choose a reason for hiding this comment

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

I think that can be done separately. I was going to ask for the newly-added action to be changed to the new format, but I didn't because the existing action would also need to be changed in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed through private messages, It was decided that this will be done in as a separate ticket and separate pull request.

@mattermod
Copy link
Contributor

Mattermost test server updated with git commit c76e151c4814472b8edce85450bfee01953d277a.

Access here: https://mattermost-webapp-pr-3207.test.mattermost.cloud

@cpanato
Copy link
Contributor

cpanato commented Aug 27, 2019

will remove the test server to do some maintenance in the cloud infra. will create a new test server after that

@cpanato cpanato removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Aug 27, 2019
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.

Added unit test.

Looks good to me now, thank you @hahmadia for your contribution!

@saturninoabril saturninoabril added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Aug 27, 2019
@saturninoabril saturninoabril merged commit 62b8069 into mattermost:master Aug 27, 2019
@cpanato cpanato added Setup Cloud Test Server Setup a test server using Mattermost Cloud and removed Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Aug 27, 2019
@mattermod
Copy link
Contributor

Test server creation failed. See the logs for more information.

@hahmadia hahmadia deleted the MM-12068 branch August 27, 2019 15:19
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Needed Requires documentation labels Sep 17, 2019
@lindy65 lindy65 added Tests/Done Release tests have been written and removed 4: Reviews Complete All reviewers have approved the pull request labels Oct 2, 2019
skheria pushed a commit to uber-archive/mattermost-webapp that referenced this pull request Oct 3, 2019
* [MM-12068] Added the ability to remove custom branding image

* [MM-12068] Fixed lint error

* [MM-12068] Update en.json

* [MM-12068] Added styling that was accidently removed

* [MM-12068] Addressed PR comments

* [MM-12068] Fixed test case

* [MM-12068] Addressed PR comments

* [MM-12068] Fixed Asyncrhonous behaviour

* [MM-12068] Add function comments

* [MM-12068] Removed unnecessary code

* [MM-12068] Fixed small code discrepancy

* [MM-12068] Generalised code

* [MM-12068] Address comments regarding incorrect ref handling and made more generalised

* [MM-12068] Lint error fixes

* [MM-12068] Addressed PR and used function pass instead of refs

* [MM-12068] return error as object and refactor code

* [MM-12068] Add unregister save action function

* [MM-12068] Lint and snapshot fixes

* add unit test
skheria pushed a commit to uber-archive/mattermost-webapp that referenced this pull request Oct 3, 2019
* [MM-12068] Added the ability to remove custom branding image

* [MM-12068] Fixed lint error

* [MM-12068] Update en.json

* [MM-12068] Added styling that was accidently removed

* [MM-12068] Addressed PR comments

* [MM-12068] Fixed test case

* [MM-12068] Addressed PR comments

* [MM-12068] Fixed Asyncrhonous behaviour

* [MM-12068] Add function comments

* [MM-12068] Removed unnecessary code

* [MM-12068] Fixed small code discrepancy

* [MM-12068] Generalised code

* [MM-12068] Address comments regarding incorrect ref handling and made more generalised

* [MM-12068] Lint error fixes

* [MM-12068] Addressed PR and used function pass instead of refs

* [MM-12068] return error as object and refactor code

* [MM-12068] Add unregister save action function

* [MM-12068] Lint and snapshot fixes

* add unit test
justinegeffen added a commit to mattermost/docs that referenced this pull request Oct 10, 2019
Added step to remove custom branding: mattermost/mattermost-webapp#3207.
justinegeffen added a commit to mattermost/docs that referenced this pull request Oct 10, 2019
Added information about removing custom brand images: mattermost/mattermost-webapp#3207.
justinegeffen added a commit to mattermost/docs that referenced this pull request Oct 10, 2019
Added information about removing custom brand images: mattermost/mattermost-webapp#3207.
@amyblais amyblais added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Oct 10, 2019
wiersgallak pushed a commit to mattermost/docs that referenced this pull request Oct 10, 2019
Added information about removing custom brand images: mattermost/mattermost-webapp#3207.
amyblais added a commit to mattermost/docs that referenced this pull request Oct 17, 2019
* Update conf.py

* Update open-source-components.rst (#3028)

* Update advanced-permissions.rst (#3037)

Added guest role

* Update branding.rst (#3046)

Added information about removing custom brand images: mattermost/mattermost-webapp#3207.

* Update searching.rst (#3048)

mattermost/mattermost#11196

* Update bulk-export-data.rst (#3036)

* Update bulk-export-data.rst

* Update source/administration/bulk-export-data.rst

Co-Authored-By: Amy Blais <amy_blais@hotmail.com>

* Update source/administration/bulk-export-data.rst

Co-Authored-By: Amy Blais <amy_blais@hotmail.com>

* Update config-settings.rst (#3052)

* Update config-settings.rst

* Update config-settings.rst

* Update source/administration/config-settings.rst

Co-Authored-By: Jason Blais <13119842+jasonblais@users.noreply.github.com>

* Update config-settings.rst

* Update config-settings.rst

* Update config-settings.rst

* Update source/administration/config-settings.rst

* Add telemetry docs for 5.16 (#3054)

* Update managing-members.rst (#3060)

* Update managing-members.rst

* Update managing-members.rst

* Update organizing-conversations.rst (#3061)

* Update organizing-conversations.rst

* Update organizing-conversations.rst

* Update important upgrade notes (#3006)

* Update important-upgrade-notes.rst

* Update important-upgrade-notes.rst

* Update desktop notification blurb

* Update important-upgrade-notes.rst

* Create guest-accounts.rst (#3050)

* Create guest-accounts.rts

* Add files via upload

* Update guest-accounts.rts

* Update administrator.rst

* Rename guest-accounts.rts to guest-accounts.rst

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update managing-members.rst

* Update guest-accounts.rst

* Update managing-members.rst

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Justine Geffen <justinegeffen@users.noreply.github.com>

* Update managing-members.rst

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Amy Blais <amy_blais@hotmail.com>

* Update guest-accounts.rst

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Amy Blais <amy_blais@hotmail.com>

* Update managing-members.rst

* Update guest-accounts.rst

* Update managing-members.rst

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Amy Blais <amy_blais@hotmail.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Amy Blais <amy_blais@hotmail.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Amy Blais <amy_blais@hotmail.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Amy Blais <amy_blais@hotmail.com>

* Update source/help/getting-started/managing-members.rst

* Update source/help/getting-started/managing-members.rst

* Update managing-members.rst

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Jason Blais <13119842+jasonblais@users.noreply.github.com>

* Update source/help/getting-started/managing-members.rst

Co-Authored-By: Jason Blais <13119842+jasonblais@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Jason Blais <13119842+jasonblais@users.noreply.github.com>

* Update source/deployment/guest-accounts.rst

Co-Authored-By: Jason Blais <13119842+jasonblais@users.noreply.github.com>

* Update guest-accounts.rst

* Update managing-members.rst

* Update guest-accounts.rst

* Add two new perf monitoring metrics (for 5.16) (#3055)

* Add two new perf monitoring metrics (for 5.16)

The memory cache invalidation isn't new, but was missed in docs.

DB Store total metric was via mattermost/enterprise#471

* Fix mistake in metrics definition (total > time)

* Update source/deployment/metrics.rst

Co-Authored-By: Amy Blais <amy_blais@hotmail.com>

* Plugin Marketplace documentation

* Update command-line-tools.rst (#3063)

* Update command-line-tools.rst

Added integrity command: mattermost/mattermost#11599

* Update source/administration/command-line-tools.rst

Co-Authored-By: Amy Blais <amy_blais@hotmail.com>

* Update source/administration/command-line-tools.rst

Co-Authored-By: Jason Blais <13119842+jasonblais@users.noreply.github.com>

* Update source/administration/command-line-tools.rst

* Update source/administration/command-line-tools.rst

Co-Authored-By: Amy Blais <amy_blais@hotmail.com>

* Update command-line-tools.rst

* v5.16 Changelog (#2992)

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Changed Marketplace highlight text 

Clarified what it can do and the value it provides

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Update changelog.md

* Adding note about desktop notifications

* Update changelog.md

* Update changelog.md

* v5.16 docs (#3067)

* Update code formatting options

* Update configuring-notifications.rst

* Update account-settings.rst

* Update prod-windows-2012.rst

* Update requirements.rst

* Update software-requirements.rst

* Update requirements.rst

* Update desktop.rst

* Update desktop-msi-gpo.rst

* updated away status

* Update configuring-notifications.rst

* Update desktop-msi-gpo.rst

* Update configuring-notifications.rst

* Update formatting-text.rst

* Added Plugin Marketplace settings (2)

EnableMarketplace and MarketplaceUrl

* Update config-settings.rst

* Update desktop.rst
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Changelog/Done Required changelog entry has been written Docs/Done Required documentation has been written Tests/Done Release tests have been written
Projects
None yet