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

Hide pre-release features section of advanced settings pane. #333

Merged
merged 2 commits into from
Nov 30, 2017
Merged

Hide pre-release features section of advanced settings pane. #333

merged 2 commits into from
Nov 30, 2017

Conversation

csduarte
Copy link
Contributor

Summary

Hide pre-release feature section of advance settings pane
Here’s the PR for the server: mattermost/mattermost#7847

Cc @dmeza

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit and component tests passed
  • Added or updated unit tests (required for all new features)
  • Has server changes (please link)
  • Has redux changes (please link)
  • Has UI changes
  • Includes text changes and localization file (.../i18n/en.json) updates
  • Touches critical sections of the codebase (auth, posting, etc.)

@dmeza dmeza mentioned this pull request Nov 20, 2017
Merged
7 tasks
@jasonblais jasonblais added 1: PM Review Requires review by a product manager Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) labels Nov 20, 2017
@jwilander jwilander removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Nov 21, 2017
@jwilander
Copy link
Member

@esethna server PR has been merged

@esethna esethna added the Setup Old Test Server Triggers the creation of a test server label Nov 27, 2017
@mattermod
Copy link
Contributor

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

@mattermod
Copy link
Contributor

Spinmint test server created at: http:https://i-031c85c1ef3f8b02a.spinmint.com

Test Account 1: Email: [email protected] | Password: passwd

Test Account 2: Email: [email protected] | Password: passwd

Instance ID: i-031c85c1ef3f8b02a

@esethna
Copy link
Contributor

esethna commented Nov 27, 2017 •
edited
Loading

@dmeza @jwilander can we set the spinmint config flag for this to "false" to test it? It's EnablePreviewFeatures

@esethna esethna added the Awaiting Submitter Action Blocked on the author label Nov 27, 2017
@jwilander jwilander removed the Awaiting Submitter Action Blocked on the author label Nov 27, 2017
@jwilander
Copy link
Member

I will update the spinmint

@jwilander
Copy link
Member

Done

@esethna esethna added 2: Dev Review Requires review by a core commiter and removed 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Nov 29, 2017
@mattermod
Copy link
Contributor

Spinmint test server destroyed

@esethna esethna removed their assignment Nov 29, 2017
esethna
esethna reviewed Nov 29, 2017
View reviewed changes
Copy link
Contributor

@esethna esethna left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks!

esethna
esethna approved these changes Nov 29, 2017
View reviewed changes
Copy link
Contributor

@esethna esethna left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good.

jwilander
jwilander reviewed Nov 29, 2017
View reviewed changes
Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

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

LGTM other than my minor comment

components/user_settings/user_settings_advanced.jsx Outdated
previewFeaturesSectionDivider = (
<div className='divider-light'/>
);
if (this.state.previewFeaturesEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we combine this if statement with the one below? Gets rid of a bit of unneeded nesting and makes way easier to read

Copy link
Contributor

Choose a reason for hiding this comment

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

@jwilander rebased to latest and combined ifs to remove unneeded nesting.

mkraft
mkraft suggested changes Nov 30, 2017
View reviewed changes
components/user_settings/user_settings_advanced.jsx
@@ -77,11 +77,15 @@ export default class AdvancedSettingsDisplay extends React.Component {

const isSaving = false;

return {preReleaseFeatures: PreReleaseFeatures,
const previewFeaturesEnabled = global.window.mm_config.EnablePreviewFeatures === 'true';
Copy link
Contributor

Choose a reason for hiding this comment

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

The default.json uses "EnablePreviewFeatures": true (with a Boolean value) but this is comparing against a string, and in JavaScript:

'true' === true // false

We should either compare against a Boolean only or both a Boolean and a string.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately we do this everywhere because the server returns the client config as a JSON object with string values (an oversight from very early in the project)

We will eventually fix it but no one's gotten around to it yet

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh OK. I didn't realized. Thanks.

mkraft
mkraft approved these changes Nov 30, 2017
View reviewed changes
Copy link
Contributor

@mkraft mkraft left a comment

Choose a reason for hiding this comment

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

👍

@jwilander jwilander added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Nov 30, 2017
@jwilander jwilander merged commit 20f4e61 into mattermost:master Nov 30, 2017
@lindalumitchell lindalumitchell added the Tests/Done Release tests have been written label Dec 1, 2017
@esethna esethna added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Dec 4, 2017
@dmeza dmeza deleted the bash-19_hide-pre-release branch December 7, 2018 18:28
hmhealey pushed a commit that referenced this pull request Aug 28, 2020
@sudheerDev @hmhealey
Fix selectors for initial load of create_post redux container as post…
1bda838 
…s will not be available on first call (#333)
hmhealey pushed a commit that referenced this pull request Mar 17, 2021
@sudheerDev @jwilander
Fix selectors for initial load of create_post redux container as post…
fb083f6 
…s will not be available on first call (#333)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dmeza dmeza dmeza left review comments

@mkraft mkraft mkraft approved these changes

@jwilander jwilander jwilander approved these changes

@esethna esethna esethna approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
Milestone
No milestone
9 participants
@csduarte @jwilander @mattermod @esethna @dmeza @mkraft @jasonblais @lindalumitchell @jarredwitt