Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BASH-11 Open help link from config #593

Merged
merged 2 commits into from
Oct 12, 2017
Merged

Conversation

csduarte
Copy link
Contributor

@csduarte csduarte commented Sep 8, 2017

Description
This PR changes the behavior of the Help menu Learn More item to open the link that is defined for the helpLink property in the base.json file (src/common/config/base.json). This property can be overridden by defining the helpLink property in the override.json file (src/common/config/override.json).

Test Cases
Add override values like this:

// Exampleoverride.js
{
  "1": {
    "helpLink": “http://test.helplink.com”
  }
}

Notes
includes BASH-20

@jasonblais jasonblais added this to the v3.8.0 milestone Sep 15, 2017
@jasonblais
Copy link
Contributor

I don't see any issues with this particular change. Makes a lot of sense. We allow overriding our help docs on the server-side.

Added the Pending label for now given the discussion in #586,

@jasonblais
Copy link
Contributor

jasonblais commented Sep 15, 2017

Actually, a question: what happens if helpLink is blank? Does it disappear from the Help Menu?

@csduarte
Copy link
Contributor Author

@jasonblais It will open the link regardless, the value at the moment. So it would link to nothing, but the default is set MM

@jasonblais
Copy link
Contributor

@csduarte @dmeza Would it be okay to ask that if the value is blank, then the option isn't shown in the Help Menu? Otherwise it would appear as a bug if nothing actually happens.

It's also consistent with how we handle help link settings server-side.

If you feel this is feature creep, let us know. We can also make this change ourselves.

@dmeza
Copy link
Contributor

dmeza commented Sep 18, 2017

@jasonblais it should not be a problem to hide if the value is blank, sounds good to have consistency.

@dmeza
Copy link
Contributor

dmeza commented Sep 21, 2017

@jasonblais @yuya-oc if help link is blank in config file, then the option isn't shown in the Help Menu:
screen shot 2017-09-20 at 10 48 42 pm

@jasonblais jasonblais requested a review from yuya-oc September 21, 2017 12:20
@yuya-oc
Copy link
Contributor

yuya-oc commented Sep 21, 2017

@dmeza Looks good. I think you haven't pushed the code yet. If you feel it's done, would you push it?

@dmeza
Copy link
Contributor

dmeza commented Sep 21, 2017

@yuya-oc pushed.

@yuya-oc
Copy link
Contributor

yuya-oc commented Sep 21, 2017

@jasonblais
Copy link
Contributor

Thanks @yuya-oc and @dmeza , looks good! Should be good to merge after #586

PS: @yuya-oc the builds failed, anything to worry about?

@yuya-oc
Copy link
Contributor

yuya-oc commented Sep 21, 2017

@jasonblais Nothing. Artifacts were correctly build.
Recently spectron tests are unstable somehow. I have been researching that, but I haven't found any good way yet.

@jasonblais
Copy link
Contributor

#586 is now merged. @dmeza can you help with a rebase, resolving merge conflicts?

This should be otherwise good to merge unless @yuya-oc has any other comments.

Copy link
Contributor

@yuya-oc yuya-oc left a comment

Choose a reason for hiding this comment

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

Due to merging of #586, please rebase the branch.

@dmeza
Copy link
Contributor

dmeza commented Oct 11, 2017

@yuya-oc rebased and tested.

@yuya-oc
Copy link
Contributor

yuya-oc commented Oct 12, 2017

https://circleci.com/gh/mattermost/desktop/1064#artifacts

The feature correctly works. By using following override.json, the link disappears.

{
  "1": {
    "helpLink": null
  }
}

@yuya-oc yuya-oc added the All Platforms null label Oct 12, 2017
@yuya-oc yuya-oc merged commit 189c07d into mattermost:master Oct 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants