Fixes #21737 - move about page to react #5024

amirfefer · 2017-11-22T16:08:05Z

About page uses 3 different old js files in order to check the status of compute resources and smart proxies.
I've extracted this status rendering to a react component <About>, in addition, I've united the UI style (by using Icons instead of labels).

new components from pf-react:

Empty State
Tabs
Table

Todo

tests

updated gif (March) :

before:

theforeman-bot · 2017-11-22T16:08:38Z

Issues: #21737

ohadlevy

nice @amirfefer - first review round..

ohadlevy · 2017-11-23T07:14:10Z

app/views/about/index.html.erb

-            </table>
-          <% end %>
+          <%= data = @smart_proxies.map {|proxy| {:id => {:name => proxy.name, :id => proxy.id},
+                                                  :features => h(proxy.features.map(&:name).to_sentence)}} %>


h is probably no longer required (it's the default).

ohadlevy · 2017-11-23T07:15:21Z

app/views/about/index.html.erb

-                </tbody>
-              </table>
-            <% end %>
+            <%= data = @compute_resources.map {|compute| {:id => {:name => compute.name, :id => compute.id},


when there are none, you get a generic message ````No data found``` - we should use the PF empty state instead.

ohadlevy · 2017-11-23T07:16:04Z

config/routes.rb

@@ -404,7 +404,7 @@
          post 'template_selected'
          post 'cluster_selected'
          get 'resource_pools'
-          post 'ping'
+          get 'ping'


doesnt this break CR test connection?

ohadlevy · 2017-11-23T07:16:51Z

webpack/assets/javascripts/react_app/common/helpers.js

+    return `/${controller}/${id ? `${id}/` : ''}${action}`;
+  },
+  getQueryParam(query) {
+    const urlParams = new URLSearchParams(window.location.search);


how is this different from the urijs lib that we currently use? - also does this work across browsers?

It becomes a standard interface, most of the browsers support it, and there's a polyfill

should we replace than the urijs?

ohadlevy · 2017-11-23T07:18:57Z

webpack/assets/javascripts/react_app/components/about/compute/index.js

+  render() {
+    const columns = [
+      {
+        Header: 'Name',


please extract all strings

ohadlevy · 2017-11-23T07:32:19Z

webpack/assets/javascripts/react_app/components/common/table/index.js

+                ? rowData
+                : undefined,
+            style: {
+              textAlign: 'center',


why pass style here?

ohadlevy · 2017-11-23T07:33:00Z

webpack/assets/javascripts/react_app/components/componentRegistry.js

@@ -8,6 +8,10 @@ import PowerStatus from './hosts/powerStatus/';
 import NotificationContainer from './notifications/';
 import ToastsList from './toastNotifications/';
 import StorageContainer from './hosts/storage/vmware/';
+import AboutComputeTable from './about/compute';


TBH: I am not too happy about having so many entry level components, is there any chance we can take over multiple tabs in one hoc ? this will allow to have all of the "logic" in one place vs across multiple components

ohadlevy · 2017-11-23T07:34:27Z

webpack/assets/javascripts/react_app/redux/actions/status/index.js

@@ -0,0 +1,12 @@
+import { STATUS_REQUEST, STATUS_SUCCESS, STATUS_FAILURE } from '../../consts';


shouldnt these const be more specific? e.g. HOST_STATUS or COMPUTE_RESOURCE_STATUS?

I've extracted it out of the tree, something like this:

STATUS
-COMPUTE_RESOURCE
[-X...]
-SMART_PROXY
[-Y...]

ohadlevy · 2017-11-23T07:35:25Z

webpack/assets/javascripts/react_app/redux/reducers/status/index.js

+
+  switch (action.type) {
+    case STATUS_REQUEST:
+      return state;


why? shouldn't this set the state so that the request has started?

ohadlevy · 2017-11-23T07:36:20Z

webpack/assets/javascripts/react_app/redux/reducers/status/index.js

+    case STATUS_REQUEST:
+      return state;
+    case STATUS_SUCCESS:
+      return state.set(payload.id, { ...payload });


I'm a bit worried that ID's can override each other, while I've seen you've prefixed it, the fact is that these actions are used for multiple objects, can confuse a future developer?

ohadlevy · 2017-11-23T07:42:18Z

@amirfefer also note the lack of tests + current failing tests.

sharvit · 2017-11-23T08:10:30Z

@amirfefer It's is great to see more and more code moving to react.
I would love to see the whole about/index.html.erb moving to react, including the tabs and the layout.
Then, you will need to add only 1 component to the registry (instead of 4).

You can use react-bootstrap for the tabs:
https://react-bootstrap.github.io/components.html#tabs

The row and col can be replaced with:
https://react-bootstrap.github.io/components.html#grid

Those components will be apart of patternfly-react in the future so it will be easy to move them later, probably by only change the import statement.

ohadlevy · 2017-11-23T08:22:01Z

app/views/about/index.html.erb

-              <% @smart_proxies.each do |proxy| %>
-                <tr class="proxy-show"  data-url="<%= ping_smart_proxy_path(proxy) %>">
-                  <td><%= link_to_if_authorized proxy.name, hash_for_smart_proxy_path(proxy) %></td>
-                  <td><%=h proxy.features.map(&:name).to_sentence %></td>


I wonder how translation worked here before?

amirfefer · 2017-11-28T18:12:58Z

@ohadlevy
I've changed the table library to react-bootstrap-table for an easy integration with pf styling. (react-table doesn't use the regular table, th, td tr tags)
I've created a new Tabs component, so there's only one registered component (instead of 4)
I've implemented a new empty state component that renders when no data instead of a no data string.

empty state component:

bootsrap & pf table style:

ohadlevy · 2017-11-28T21:16:28Z

@amirfefer i think you forgot to push your new implementation.

regardless, I would suggest to send PR's to patternfly-react repo with all of pf related components (tabs, emptystate etc)

ohadlevy

I ran out of time, but here is some feedback

ohadlevy · 2017-11-30T13:36:42Z

app/assets/stylesheets/base.scss

@@ -250,7 +250,7 @@ select {

 table {
  clear: both;
-  margin-bottom: 6px !important;
+  //margin-bottom: 6px !important;


ohadlevy · 2017-11-30T13:37:06Z

app/assets/stylesheets/base.scss

@@ -335,7 +335,7 @@ table {
 }

 #pagination {
-  margin-top: -6px;
+  margin-top: -19px !important;


why change style for all pagination?

ohadlevy · 2017-11-30T13:38:34Z

app/helpers/about_helper.rb

@@ -0,0 +1,31 @@
+module AboutHelper
+  def plugins


if you are creating a controller per action, then I would expect AboutPluginController with only index action.

having said that, I would default to use the API first and only if it doesnt fit create a UI end point

I've created About controller namespace, inspired by this post.

in that case why do you need the helper than?
also.. can't we simply use the existing api?

Using an api endpoint would definitely be more systemic. Such endpoint doesn't exist at the moment. We can only list proxies and compute profiles separately. How about moving it to a new api endpoint? It would help with http://projects.theforeman.org/issues/3036.

But I understand it's adds some work and there are probably issues with higher priority. So if you don't have space for moving it to API, I can live with the current state ;)

ohadlevy · 2017-11-30T13:40:08Z

app/views/about/index.html.erb

-        </div>
-      </div>
+      <div id="tabs"></div>   
+      <%= mount_react_component('About', '#tabs', {:compute => compute_resources, :proxy => proxies,


probably makes sense to combine it under a method, so in theory we can expand it later vs hard code it. e.g.

def about_data {:compute => compute_resources, :proxy => proxies,... end

ohadlevy · 2017-11-30T13:41:51Z

package.json

@@ -68,12 +68,13 @@
    "lodash": "~4.15.0",
    "multiselect": "~0.9.12",
    "patternfly": "^3.29.5",
-    "patternfly-react": "^0.8.1",
+    "patternfly-react": "^0.11.0",


all package upgrades seems not related?

I've upgraded in order to use Icon component

ohadlevy · 2017-11-30T13:42:17Z

webpack/assets/javascripts/react_app/common/helpers.js

+    return `/${controller}/${id ? `${id}/` : ''}${action}`;
+  },
+  getQueryParam(query) {
+    const urlParams = new URLSearchParams(window.location.search);


should we replace than the urijs?

ohadlevy · 2017-11-30T13:42:46Z

webpack/assets/javascripts/react_app/components/about/compute/index.js

+  render() {
+    const columns = [
+      {
+        header: 'Name',


string not extracted?

extracted already in <Table> component

ohadlevy · 2017-11-30T13:43:57Z

webpack/assets/javascripts/react_app/components/about/index.js

+    } = this.props.data;
+    const tabs = [__('Smart Proxies'), __('Available Providers'), __('Compute Resources'), __('Plugins')];
+
+    return <ForemanTabs id='about_tabs' tabs={tabs} >


this looks really close to how react-router works.. I wonder if we should combine the two? (so if you refresh you come back to the same tab)

sounds great, though this PR becomes too large, maybe should we combine react-router in a different PR?

ohadlevy · 2017-11-30T13:45:15Z

webpack/assets/javascripts/react_app/components/common/emptyState/index.js

+        </p>
+        <div className="blank-slate-pf-main-action">
+          <button
+            onClick={e => window.Turbolinks.visit(action.url)}


wouldnt turbolink catch these regardless?

ohadlevy · 2017-11-30T13:45:26Z

webpack/assets/javascripts/react_app/components/common/emptyState/index.js

+        <h1>{header}</h1>
+        <p>{description}</p>
+        <p>
+          Learn more about this{' '}


not extracted..

houndci-bot · 2017-12-05T11:20:21Z

webpack/assets/javascripts/react_app/redux/reducers/index.js


+<<<<<<< HEAD


Parsing error: Unexpected token

houndci-bot · 2017-12-05T11:20:21Z

app/assets/stylesheets/base.scss

@@ -655,3 +655,7 @@ code.transparent {
  padding: 0;
  margin-bottom: 0;
 }
+
+.react-bs-table table {
+  margin-bottom: 0px !important


Unexpected unit length-zero-no-unit
Unexpected missing end-of-source newline no-missing-end-of-source-newline
Expected a trailing semicolon declaration-block-trailing-semicolon

houndci-bot · 2017-12-05T11:20:21Z

app/assets/stylesheets/base.scss

@@ -239,7 +239,7 @@ select {

 table {
  clear: both;
-  margin-bottom: 6px !important;
+  margin-bottom: 6px !important;  


Expected newline after ";" in a multi-line declaration block declaration-block-semicolon-newline-after
Unexpected whitespace at end of line no-eol-whitespace

houndci-bot · 2017-12-05T11:38:26Z

app/assets/stylesheets/base.scss

@@ -655,3 +655,7 @@ code.transparent {
  padding: 0;
  margin-bottom: 0;
 }
+
+.react-bs-table table {
+  margin-bottom: 0 !important;


Unexpected missing end-of-source newline no-missing-end-of-source-newline

tstrachota · 2018-04-30T08:16:10Z

@amirfefer ping, could you rebase, please?

xprazak2 · 2018-05-25T12:01:01Z

@amirfefer, could you rebase please?

tstrachota · 2018-06-20T13:09:54Z

@amirfefer ping

amirfefer · 2018-06-20T19:16:20Z

It was among the latter in the priority tasks list,
I'll rebase and have a look on it soon.

amirfefer · 2018-06-21T15:24:22Z

@tstrachota , @waldenraines
Rebased, and ready for a another look :)

boaz0 · 2018-06-28T19:50:01Z

webpack/assets/javascripts/react_app/components/common/table/index.js

+    return this.isEmpty() ? (
+      <EmptyState {...emptyState} />
+    ) : (
+      <PfTable.PfProvider


Hi @amirfefer, this looks good but what if I want to use components in <PfTable.PfProvider> - I am not sure I can do that through Table. In Hardware Models I am using it to set how the heading and cells will look like.

This has been solved in Katello: https://github.com/Katello/katello/blob/master/webpack/move_to_foreman/components/common/table/components/Table.js That component is based on @amirfefer 's solution and is going to be moved to foreman soon. AFAIR the interfaces should be compatible.
So, if you don't mind @ripcurld, I wouldn't block merging this PR by that. It contains lot of other good stuff, has been open for quite some time and is quite ready to be merged.

@tstrachota I didn't mean to block 😃 but just to raise a concern I have in Hardware Models.

tstrachota · 2018-06-30T21:53:37Z

There's minor conflict again. I wasn't fast enough with the final review:( I'm sorry for that. Please rebase @amirfefer

amirfefer · 2018-07-03T15:50:08Z

@tstrachota - rebased :)

tstrachota

I found two more issues on the empty states. Hopefully they'll be easy to fix.
The rest looks good and worked fine for me.

tstrachota · 2018-07-09T13:54:32Z

webpack/assets/javascripts/react_app/common/EmptyStates.js

+  description: __('Foreman supports creating and managing hosts on a number of virtualization and cloud services - referred to as “compute resources” - as well as bare metal hosts.'),
+  documentation: {
+    // eslint-disable-next-line no-undef
+    url: `https://www.theforeman.org/manuals/${VERSION}/index.html#5.2ComputeResources`,


I found out that the docs link (and the other too) leads to 404. VERSION is 1.19.0 but the manual url has to contain only 1.19. We probably need both VERSION and VERSION_MAJOR or something similar, maybe an object containing multiple version formats and with toString() defined.

tstrachota · 2018-07-09T14:28:39Z

webpack/assets/javascripts/react_app/components/common/emptyState/index.js

+      {url &&
+        <PfEmptyState.Help>
+         {label}
+          <a href={url}> {buttonLabel} </a>


Nitpick:
this indend doesn't put any space between the text and the link. Spaces in <a> work but they look weird on hover:

Please indent correctly and remove spaces form the anchor. {" "} should help with preserving the space in between.

mmoll · 2018-08-15T00:03:46Z

@amirfefer could you rebase and fix the last two small comments from @tstrachota? :)

mmoll · 2018-09-01T13:52:24Z

what's the status here?

tstrachota · 2018-10-24T13:28:46Z

This needs a rebase again.

waldenraines · 2018-11-12T20:01:55Z

@amirfefer any updates on this?

amirfefer · 2018-11-14T15:11:57Z

@waldenraines - basically i'd like to use table from this PR, will re-prioritize this soon :)

mmoll · 2019-04-06T16:09:27Z

@amirfefer what's the status here?

mmoll · 2019-05-16T21:53:19Z

any news here? :)

ares · 2019-07-24T08:36:25Z

@amirfefer this needs a bigger rebase now, could you please let us know if you plan to look at this soon? If there's no activity before next triage (2 weeks), we'd lean towards closing this. It would be unfrotunate as there were just 2 small things to finish this, but of course it can be reopened later if your availability for this PR changes.

ares · 2019-08-07T08:09:39Z

Unfortunatelly there was no reply for 2 weeks so we are closing this PR now. Please reopen if you're still interested in getting this in.

theforeman-bot added Needs testing Not yet reviewed labels Nov 22, 2017

amirfefer force-pushed the 21737 branch 2 times, most recently from 5ed7f4c to 22e7fec Compare November 22, 2017 16:46

ohadlevy suggested changes Nov 23, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Nov 23, 2017

ohadlevy reviewed Nov 23, 2017

View reviewed changes

amirfefer force-pushed the 21737 branch from 22e7fec to 70254e2 Compare November 30, 2017 10:23

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 30, 2017

amirfefer force-pushed the 21737 branch from 70254e2 to 766996a Compare November 30, 2017 10:28

sharvit mentioned this pull request Nov 30, 2017

Component - Empty State Pattern patternfly/patternfly-react#82

Closed

ohadlevy suggested changes Nov 30, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Nov 30, 2017

sharvit mentioned this pull request Dec 5, 2017

Fixes #21203 - Move from webpack-rails to webpacker #5033

Closed

2 tasks

amirfefer force-pushed the 21737 branch from 766996a to db60055 Compare December 5, 2017 11:19

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Dec 5, 2017

houndci-bot reviewed Dec 5, 2017

View reviewed changes

amirfefer force-pushed the 21737 branch from db60055 to 932ff8f Compare December 5, 2017 11:38

houndci-bot reviewed Dec 5, 2017

View reviewed changes

amirfefer force-pushed the 21737 branch from c0c59da to c1a6ed5 Compare June 21, 2018 15:22

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jun 21, 2018

amirfefer force-pushed the 21737 branch from c1a6ed5 to f664e49 Compare June 21, 2018 16:34

boaz0 reviewed Jun 28, 2018

View reviewed changes

waldenraines mentioned this pull request Jun 29, 2018

Fixes #24041 - Move HW Models Table to pf-react #5755

Merged

Fixes #21737 - move about page to react

782d1f4

amirfefer force-pushed the 21737 branch from f664e49 to 782d1f4 Compare July 3, 2018 15:49

tstrachota suggested changes Jul 9, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Jul 9, 2018

boaz0 mentioned this pull request Jul 18, 2018

Fixes #24475 - Add EmptyState to React components #5850

Merged

ares closed this Aug 7, 2019

		@@ -0,0 +1,12 @@
		import { STATUS_REQUEST, STATUS_SUCCESS, STATUS_FAILURE } from '../../consts';


		<<<<<<< HEAD

Fixes #21737 - move about page to react #5024

Fixes #21737 - move about page to react #5024

Conversation

amirfefer commented Nov 22, 2017 • edited Loading

new components from pf-react:

Todo

updated gif (March) :

before:

theforeman-bot commented Nov 22, 2017

ohadlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy commented Nov 23, 2017

sharvit commented Nov 23, 2017

Choose a reason for hiding this comment

amirfefer commented Nov 28, 2017 • edited Loading

ohadlevy commented Nov 28, 2017

ohadlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstrachota commented Apr 30, 2018

xprazak2 commented May 25, 2018

tstrachota commented Jun 20, 2018

amirfefer commented Jun 20, 2018

amirfefer commented Jun 21, 2018

boaz0 Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstrachota commented Jun 30, 2018

amirfefer commented Jul 3, 2018

tstrachota left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmoll commented Aug 15, 2018

mmoll commented Sep 1, 2018

tstrachota commented Oct 24, 2018

waldenraines commented Nov 12, 2018

amirfefer commented Nov 14, 2018 • edited Loading

mmoll commented Apr 6, 2019

mmoll commented May 16, 2019

ares commented Jul 24, 2019

ares commented Aug 7, 2019

amirfefer commented Nov 22, 2017 •

edited

Loading

amirfefer commented Nov 28, 2017 •

edited

Loading

amirfefer Dec 5, 2017 •

edited

Loading

boaz0 Jun 28, 2018 •

edited

Loading

amirfefer commented Nov 14, 2018 •

edited

Loading