-
Notifications
You must be signed in to change notification settings - Fork 154
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
[MM-36] Update plugin with respect to phase 1 upgrades #727
Conversation
@ayusht2810 Can you please merge master into your PR? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #727 +/- ##
==========================================
+ Coverage 15.85% 15.93% +0.08%
==========================================
Files 15 17 +2
Lines 5778 6018 +240
==========================================
+ Hits 916 959 +43
- Misses 4820 5017 +197
Partials 42 42 ☔ View full report in Codecov by Sentry. |
/update-branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One blocker, otherwise LGTM
Let's merge #725 first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the basic flows and slash commands, everything working fine. Approved.
cc @AayushChaudhary0001
#192) * Revert "Update main.go (#154)" This reverts commit be4a281d0cc791d10e6e5ae917b325b2f054e475. * Revert "[MM-33506] Use embed package to include plugin manifest (#145)" This reverts commit ca9ee3c17c6920a636a33f378e17395afd6f329f. * Revert "Don't generate manifest.ts (#127)" This reverts commit 18d30b50bc7ba800c9f05bfd82970781db0aea3e. * install-go-tools target, adopt gotestsum * bring back make apply + automatic versioning * Update build/manifest/main.go Co-authored-by: Michael Kochell <[email protected]> * suppress git describe error when no tags match * make version/release notes opt-in * fix whitespace in Makefile * document version management options --------- Co-authored-by: Michael Kochell <[email protected]>
Co-authored-by: Jesse Hallam <[email protected]>
@@ -16,7 +16,7 @@ const mapStateToProps = (state, ownProps) => { | |||
const systemMessage = post ? isSystemMessage(post) : true; | |||
|
|||
return { | |||
show: state[`plugins-${pluginId}`].connected && !systemMessage, | |||
show: state[`plugins-${manifest.id}`].connected && !systemMessage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required for this PR, but it would be good to always use a selector for this sort of thing, and never do this plugins-(id)
thing in these files. Can this be done as part of the redux store migration?
@@ -26,7 +26,7 @@ export default class AttachCommentToIssuePostMenuAction extends PureComponent { | |||
}; | |||
|
|||
connectClick = () => { | |||
window.open('/plugins/' + pluginId + '/user/connect', '_blank'); | |||
window.open('/plugins/' + manifest.id + '/user/connect', '_blank'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should really be using the server's SiteURL (not needed for this PR)
@@ -4,7 +4,7 @@ | |||
import React, {PureComponent} from 'react'; | |||
import PropTypes from 'prop-types'; | |||
|
|||
import {id as pluginId} from '../../../manifest'; | |||
import manifest from '../../../manifest'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see sometimes we import the manifest as 'manifest'
and sometimes with the relative path. Probably should just do 'manifest'
Summary
Updated plugin with respect to phase 1 upgrades
nvmrc
file and updatepackage-lock.json
file.node-sass
tosass
Ticket Link