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
Adding react #32
Adding react #32
Conversation
amirfefer
commented
Apr 23, 2020
•
edited
edited
- Adding Example Component
- Adding Foreman metapackages (test, lint, storybook, vendor)
- Adding React Router
- Adding Redux infra & usage
- Adding Slot&Fill infra
- Adding API usage via APIMiddleware
- Adding React Router interface from foreman
Nice work! Should the rename script process the js files as well in some way? |
d29bed1
to
f1d5f16
Compare
Thanks @xprazak2 ! ready for another review :) |
|
||
<% content_for(:content) do %> | ||
<%= notifications %> | ||
<div id="organization-id" data-id="<%= Organization.current.id if Organization.current %>" ></div> |
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.
Should ForemanContext
be used instead?
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.
Sounds like a nice to have 👍 , but it needs another PR in foreman-core for adding taxonomies into app_metadata
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've added taxonomies and user_id to the app_metadata here - theforeman/foreman#7655
Nice progress, should Gemfile.lock be included if it is in gitignore? |
@@ -0,0 +1,17 @@ | |||
module ForemanPluginTemplate | |||
class ReactController < ::ApplicationController |
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.
Should plugins reuse the ReactController
and react layout from core?
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.
When the react router for plugins will be ready in foreman-core, the core's react layout and controller will be reusable, until then we still need to mount plugin's root component via plugin's react layout.
# plugin/layout/react
mount_react_component('PluginTemplate', '#foremanPluginTemplateRoot')
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.
So can we drop this now?
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.
a couple of small comments inline, other than that lgtm.
@@ -18,9 +18,13 @@ class Engine < ::Rails::Engine | |||
Foreman::Plugin.register :foreman_plugin_template do | |||
requires_foreman '>= 1.16' | |||
|
|||
# Add Global JS file for extending foreman-core components and routes | |||
register_global_js_file 'fills' |
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.
the file is named fills_index.js
, so i think this needs to change
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.
the _index
suffix is added automatically.
@@ -18,9 +18,13 @@ class Engine < ::Rails::Engine | |||
Foreman::Plugin.register :foreman_plugin_template do | |||
requires_foreman '>= 1.16' |
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 be the first version that matches what is in the package.json
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.
Thanks @amirfefer it's great to see this PR, looks good!
added some suggestions
module ForemanPluginTemplate | ||
class ReactController < ::ApplicationController | ||
def index | ||
render 'foreman_plugin_template/layouts/react', :layout => false |
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.
can you instead add a default layout in app/views/layout/foreman_plugin_template/application.html.erb
and than set it on line 3 as the default layout: layout "foreman_plugin_template/application"
this will reduce the need to call the render method on each action as it will automatically render app/views/foreman_plugin_template/react/index.html.erb
"test:current": "tfm-test --plugin --watch", | ||
"publish-coverage": "tfm-publish-coverage", | ||
"stories": "tfm-stories --plugin", | ||
"stories:build": "tfm-build-stories --plugin", |
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.
do we also want stories:deploy
to surge?
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 think that the storybook deployment should stay consumer's choice
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.
true, but we can make it all set for them and they can decide to remove it or not,
I don't think it's too painful to have :)
webpack/index.js
Outdated
import componentRegistry from 'foremanReact/components/componentRegistry'; | ||
import { registerReducer } from 'foremanReact/common/MountingService'; | ||
import reducers from './src/reducers'; | ||
import PluginTemplate from './src/PluginName'; |
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.
should PluginName
folder be renamed to ForemanPluginTemplate
?
webpack/index.js
Outdated
// register components for erb mounting | ||
componentRegistry.register({ | ||
name: 'PluginTemplate', | ||
type: PluginTemplate, | ||
}); |
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.
can you create another component
file that contains pairs of { name, type }
and here make a forEach
loop, same as the reducers
are being registered
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.
doesn't it overkill for a very small amount of reducers? , also the reducer registering might be unclear for newcomers?
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 think that the registries will be very easy to use, as they won't even need to touch this file
and just put all of their component in one spot without calling each time componentRegistry.register()
@@ -0,0 +1,2 @@ | |||
export const ADD_CONTENT = '[PLUGIN_TEMPLATE] ADD_CONTENT'; |
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.
will the name be replaced with a script?
after it will be replaced we should keep on the underline convention.
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.
It is written in a way to make it clear for renaming, in redux console it would bold enough like that IMHO.
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.
yes, but can it be done automatically with the rename.rb
?
can you also rename this file into EmptyStateConstants.js
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.
what do you think @amirfefer ?
|
||
export const AddEmptyStateContent = header => ({ | ||
type: ADD_CONTENT, | ||
payload: header, |
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 believe the payload
should be wrapped by an object
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 it this case, the payload is just a string, it's a very simple action
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.
IMO we should create another example, maybe something with the empty state button click which creates an API call that passes some data to the page,
passing the header feels like something that doesn't happen much.
export const fetchData = url => ({ | ||
type: API_OPERATIONS.GET, | ||
key: FETCHING_KEY, // you will need to re-use this key in order to access the right API reducer later. | ||
url, | ||
payload: {}, | ||
}); |
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.
instead you can use the get
helper from Foreman, e.g:
import { get } from 'foremanReact/redux/API';
get({ key, url })
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.
Thanks, added
switch (action.type) { | ||
case ADD_CONTENT: | ||
return state.set('header', payload); | ||
|
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.
nit: extra line
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.
our linter allows this kind of spacing
webpack/src/PluginName.js
Outdated
@@ -0,0 +1,11 @@ | |||
import React from 'react'; | |||
import { BrowserRouter } from 'react-router-dom'; | |||
import PluginTemplateRoute from './Route'; |
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.
should we rename Route
to Router
?
webpack/src/Route/routes.js
Outdated
welcome: { | ||
path: '/foreman_plugin_template', | ||
exact: true, | ||
render: () => <WelcomPage />, |
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.
should we use component
instead of render
in this cases?
Thanks @LaViro for your review! |
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.
Testing it today/tomorrow, I'll add what I find :)
webpack/src/Router/routes.js
Outdated
@@ -0,0 +1,11 @@ | |||
import WelcomPage from './WelcomePage'; |
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.
import WelcomPage from './WelcomePage'; | |
import WelcomePage from './WelcomePage'; |
webpack/src/Router/routes.js
Outdated
welcome: { | ||
path: '/foreman_plugin_template', | ||
exact: true, | ||
component: WelcomPage, |
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.
component: WelcomPage, | |
component: WelcomePage, |
foreman_plugin_template.gemspec
Outdated
s.authors = ['TODO: Your name'] | ||
s.email = ['TODO: Your email'] | ||
s.homepage = 'TODO' | ||
s.summary = 'TODO: Summary of ForemanPluginTemplate.' | ||
s.authors = [' Your name'] | ||
s.email = [' Your email'] | ||
s.homepage = '' | ||
s.summary = ' Summary of ForemanPluginTemplate.' |
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.
What is the reason for this? 🤔
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.
for testing with foreman core, forgot to delete it :(
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.
Bunch of places need Foreman
prefix to get picked up by ./rename.rb
Also rename.rb
needs line system(%(sed 's/foremanPluginTemplate/#{camel_lower}/g' #{tmp_file} > #{path}))
package.json
Outdated
@@ -0,0 +1,42 @@ | |||
{ | |||
"name": "plugin-name", |
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 is not picked up by rename.rb
"name": "plugin-name", | |
"name": "foreman_plugin_template", |
webpack/index.js
Outdated
import componentRegistry from 'foremanReact/components/componentRegistry'; | ||
import { registerReducer } from 'foremanReact/common/MountingService'; | ||
import reducers from './src/reducers'; | ||
import PluginTemplate from './src/PluginTemplate'; |
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.
import PluginTemplate from './src/PluginTemplate'; | |
import ForemanPluginTemplate from './src/ForemanPluginTemplate'; |
webpack/index.js
Outdated
name: 'PluginTemplate', | ||
type: PluginTemplate, |
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.
name: 'PluginTemplate', | |
type: PluginTemplate, | |
name: 'ForemanPluginTemplate', | |
type: ForemanPluginTemplate, |
webpack/src/PluginTemplate.js
Outdated
@@ -0,0 +1,11 @@ | |||
import React from 'react'; | |||
import { BrowserRouter } from 'react-router-dom'; | |||
import PluginTemplateRoute from './Router'; |
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.
import PluginTemplateRoute from './Router'; | |
import ForemanPluginTemplateRoute from './Router'; |
webpack/src/PluginTemplate.js
Outdated
import { BrowserRouter } from 'react-router-dom'; | ||
import PluginTemplateRoute from './Router'; | ||
|
||
const PluginTemplate = () => ( |
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.
const PluginTemplate = () => ( | |
const ForemanPluginTemplate = () => ( |
webpack/src/PluginTemplate.js
Outdated
|
||
const PluginTemplate = () => ( | ||
<BrowserRouter> | ||
<PluginTemplateRoute /> |
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.
<PluginTemplateRoute /> | |
<ForemanPluginTemplateRoute /> |
webpack/src/PluginTemplate.js
Outdated
</BrowserRouter> | ||
); | ||
|
||
export default PluginTemplate; |
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.
export default PluginTemplate; | |
export default ForemanPluginTemplate; |
webpack/src/index.js
Outdated
@@ -0,0 +1 @@ | |||
export { default } from './PluginName'; |
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.
export { default } from './PluginName'; | |
export { default } from './ForemanPluginName'; |
Also the file needs renaming :)
webpack/src/reducers.js
Outdated
import EmptyStateReducer from './Components/EmptyState/EmptyStateReducer'; | ||
|
||
const reducers = { | ||
pluginTemplate: combineReducers({ |
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.
pluginTemplate: combineReducers({ | |
foremanPluginTemplate: combineReducers({ |
<div id="foremanPluginTemplateRoot"></div> | ||
<% end %> | ||
<%= render file: "layouts/base" %> | ||
<%= mount_react_component('PluginTemplate', '#foremanPluginTemplateRoot') %> |
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.
<%= mount_react_component('PluginTemplate', '#foremanPluginTemplateRoot') %> | |
<%= mount_react_component('ForemanPluginTemplate', '#foremanPluginTemplateRoot') %> |
webpack/src/Router/routes.test.js
Outdated
|
||
import Routes from './routes'; | ||
|
||
describe('PluginTemplateRoutes', () => { |
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.
describe('PluginTemplateRoutes', () => { | |
describe('ForemanPluginTemplateRoutes', () => { |
webpack/src/Router/routes.test.js
Outdated
describe('PluginTemplateRoutes', () => { | ||
it('should create routes', () => { | ||
Object.entries(Routes).forEach(([key, Route]) => { | ||
const component = shallow(<Route.render history={{}} some="props" />); |
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.
const component = shallow(<Route.render history={{}} some="props" />); | |
const component = shallow(<Route.component history={{}} some="props" />); |
Thanks @ezr-ondrej, I have updated the |
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.
Awesome work @amirfefer !
Left a few comments,
can you add more tests ?
can you also add .travis.yml
file?
ForemanModal.Header = () => jest.fn(); | ||
ForemanModal.Footer = () => jest.fn(); | ||
export default ForemanModal; | ||
``` |
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.
Looks like this file is accidentally called readme.md
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.
can you rename the file?
@@ -0,0 +1,2 @@ | |||
export const ADD_CONTENT = '[PLUGIN_TEMPLATE] ADD_CONTENT'; |
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.
yes, but can it be done automatically with the rename.rb
?
can you also rename this file into EmptyStateConstants.js
@@ -0,0 +1,2 @@ | |||
const selectEmptyState = state => state.foremanPluginTemplate.emptyState; | |||
export const selectEmptyStateHeader = state => selectEmptyState(state).header; |
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.
can you add selector tests?
const header = useSelector(selectEmptyStateHeader); | ||
|
||
useEffect(() => { | ||
dispatch(AddEmptyStateHeader(__('Foreman Plugin Template - React Page'))); |
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.
can you add an index.js
file where it will trigger the action
while passing only the header
prop into this component?
url, | ||
payload: {}, | ||
}); | ||
|
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.
can you add tests to the actions?
export default (state = initialState, action) => { | ||
const { payload } = action; | ||
|
||
switch (action.type) { |
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.
can you destruct also type
?
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.
and can we also add a test for the reducer?
const PluginTemplate = () => ( | ||
<BrowserRouter> | ||
<PluginTemplateRoute /> | ||
</BrowserRouter> |
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.
can you also add react-intl
?
"devDependencies": { | ||
"@babel/core": "^7.7.0", | ||
"@theforeman/builder": "^4.0.2", | ||
"@theforeman/eslint-plugin-foreman": "4.0.5", |
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.
can you align all of @theforeman/packages
to have the same version?
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.
can you also upgrade to 4.8.0 version of @theforeman
packages?
.stylelintrc
Outdated
@@ -0,0 +1,6 @@ | |||
|
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.
nit: extra space
@@ -0,0 +1,3 @@ | |||
module.exports = { |
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.
nit: can we do something like:
export const presets = ["@theforeman/builder/babel"];
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.
Could you please change the naming so the rename
picks it up and fix the endlines?
I belive the tests would be nice to have, but can be as a followup PR, so if you fix followings, I'd be ok to merge.
@LaViro do you have any blocking comments?
webpack/src/ForemanPluginTemplate.js
Outdated
@@ -0,0 +1,11 @@ | |||
import React from 'react'; | |||
import { BrowserRouter } from 'react-router-dom'; | |||
import PluginTemplateRoute from './Router'; |
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.
import PluginTemplateRoute from './Router'; | |
import ForemanPluginTemplateRoute from './Router'; |
webpack/src/ForemanPluginTemplate.js
Outdated
import { BrowserRouter } from 'react-router-dom'; | ||
import PluginTemplateRoute from './Router'; | ||
|
||
const PluginTemplate = () => ( |
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.
const PluginTemplate = () => ( | |
const ForemanPluginTemplate = () => ( |
webpack/src/ForemanPluginTemplate.js
Outdated
|
||
const PluginTemplate = () => ( | ||
<BrowserRouter> | ||
<PluginTemplateRoute /> |
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.
<PluginTemplateRoute /> | |
<ForemanPluginTemplateRoute /> |
webpack/src/ForemanPluginTemplate.js
Outdated
</BrowserRouter> | ||
); | ||
|
||
export default PluginTemplate; |
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.
export default PluginTemplate; | |
export default ForemanPluginTemplate; |
.gitignore
Outdated
node_modules | ||
package-lock.json | ||
Gemfile.lock | ||
coverage |
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.
missing endline
.stylelintrc
Outdated
"extends": [ | ||
"stylelint-config-standard", | ||
], | ||
} |
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.
missing endline
webpack/fills_index.js
Outdated
@@ -0,0 +1,15 @@ | |||
// This example for extanding foreman-core's component via slot&fill |
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 example for extanding foreman-core's component via slot&fill | |
// This is an example of extending foreman-core's component via slot&fill |
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.
great work @amirfefer, are you waiting for something to be merged in core?
I still have some few unresolved suggestions from the last review,
added a few more comments, thanks!
private | ||
|
||
def controller_permission | ||
:foreman_plugin_template |
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.
regarding permissions I think we should let the plugin owner to set the permissions as they are going to change anyway.. but the react layout doesn't need to change
<div id="foremanPluginTemplateRoot"></div> | ||
<% end %> | ||
<%= render file: "layouts/base" %> | ||
<%= mount_react_component('ForemanPluginTemplate', '#foremanPluginTemplateRoot') %> |
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.
can you use the react_component
helper instead? you can remove the extra div from line 13
config/routes.rb
Outdated
@@ -1,3 +1,4 @@ | |||
Rails.application.routes.draw do | |||
get 'new_action', to: 'foreman_plugin_template/hosts#new_action' | |||
get 'foreman_plugin_template', to: 'foreman_plugin_template/react#index' |
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.
can you rename the route into "welcome_page"
ForemanModal.Header = () => jest.fn(); | ||
ForemanModal.Footer = () => jest.fn(); | ||
export default ForemanModal; | ||
``` |
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.
can you rename the file?
@@ -0,0 +1,2 @@ | |||
export const ADD_CONTENT = '[PLUGIN_TEMPLATE] ADD_CONTENT'; |
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.
what do you think @amirfefer ?
}; | ||
|
||
describe('EmptyState', () => | ||
testComponentSnapshotsWithFixtures(EmptyState, fixtures)); |
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.
can you move it to the __tests__
folder?
"devDependencies": { | ||
"@babel/core": "^7.7.0", | ||
"@theforeman/builder": "^4.0.2", | ||
"@theforeman/eslint-plugin-foreman": "4.0.5", |
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.
can you also upgrade to 4.8.0 version of @theforeman
packages?
@@ -0,0 +1,11 @@ | |||
import React from 'react'; | |||
import { BrowserRouter } from 'react-router-dom'; |
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.
should we use the redux router
that is now being added to core?
We should add a |
<PfEmptyState.Info>{description}</PfEmptyState.Info> | ||
<PfEmptyState.Action> | ||
<Button | ||
href="https://theforeman.github.io/foreman" |
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.
Why does it link here? I'd always suggest to link to something like example.com
in sample code.
|
What's the status here? |
s.authors = 'a' | ||
s.email = ['a'] | ||
s.homepage = 'http://a.com' | ||
s.summary = 'a' | ||
# also update locale/gemspec.rb | ||
s.description = 'TODO: Description of ForemanPluginTemplate.' | ||
s.description = 'a' |
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 not change ^_^
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.
Can we merge the global files and drop the Plugins React controller? :)
I believe we should drop it, even if we would want some plugins to still use it, the template is for new plugins and we want to encourage the usage of the new router.
|
||
# Add Global files for extending foreman-core components and routes | ||
register_global_js_file 'fills' | ||
register_global_js_file 'routes' |
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.
Can we use one file for all of the plugin's global registrations?
@@ -16,11 +16,16 @@ class Engine < ::Rails::Engine | |||
|
|||
initializer 'foreman_plugin_template.register_plugin', :before => :finisher_hook do |_app| | |||
Foreman::Plugin.register :foreman_plugin_template do | |||
requires_foreman '>= 1.16' | |||
requires_foreman '>= 2.0.0' |
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.
We will need to bump the version if we want to use the core router.
requires_foreman '>= 2.0.0' | |
requires_foreman '>= 2.3.0' |
This PR has been refactored, due to the number of comments I'm closing this in favor of #35 |