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

boaz0 · 2018-06-29T09:35:22Z

This is almost ready -

Need to add tests to Rails. I will be working on it. In addition, I will add integration tests to ModelsTable.
I did the best I can to make the table look similar to what it looked before. However, Katello implementation didn't fit here. Probably because I am not using Pagination and Empty State is rendered by Rails now.
I didn't enter the pagination here, I will do that in the next PR. I wanted this PR to be smaller and easy to review as much as possible.

Thanks.

theforeman-bot · 2018-06-29T09:35:25Z

Issues: #24041

app/models/model.rb

waldenraines

Code looks good overall to me, just a few comments. Would be good to get @sharvit, @amirfefer, and/or @tstrachota to have a look as well.

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

webpack/assets/javascripts/react_app/components/ModelsTable/ModelsTableFormats.js

webpack/assets/javascripts/react_app/components/ModelsTable/ModelsTableSchema.js

app/models/model.rb

ohadlevy

basic comments on the rails side, didnt look at the js yet.

app/controllers/models_controller.rb

app/models/model.rb

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

webpack/assets/javascripts/react_app/components/ModelsTable/ModelsTableFormats.js

webpack/assets/javascripts/react_app/components/ModelsTable/ModelsTableReducer.js

webpack/assets/javascripts/react_app/components/ModelsTable/ModelsTableSchema.js

app/views/models/index.html.erb

ohadlevy · 2018-07-01T13:33:14Z

webpack/assets/javascripts/react_app/components/ModelsTable/ModelsTableFormats.js

+  ));
+
+const orderChars = {
+  ASC: '▲',


looking at PF documentation at https://www.patternfly.org/pattern-library/

they have the following html for table headers:

<th class="sorting_asc" tabindex="0" aria-controls="patternTable" rowspan="1" colspan="1" aria-sort="ascending" aria-label="Pattern: activate to sort column descending" > Pattern </th>

amirfefer

@ripcurld Great progress, I left some inline comments.

Looks like there is no tooltip:

you can use EllipsisWithTooltip

Foreman test failures are related: Please fix the hw-model integration test.

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

webpack/assets/javascripts/react_app/components/ModelsTable/ModelsTableSchema.js

webpack/assets/javascripts/react_app/components/ModelsTable/ModelsTableFormats.test.js

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

app/controllers/models_controller.rb

webpack/assets/javascripts/react_app/components/ModelsTable/ModelsTableActions.js

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

app/controllers/models_controller.rb

ohadlevy · 2019-02-13T15:55:07Z

test failures not related https://community.theforeman.org/t/can-we-drop-node-js-6-14-from-travis-javascript-test-matrix/12737

boaz0 · 2019-02-14T08:22:25Z

@sharvit @ohadlevy can you please take a look?
Thanks.

ohadlevy · 2019-02-14T12:57:20Z

have a look at Gilad's audit commit, as he is pretty much doing something similar.

…

On Thu, Feb 14, 2019 at 2:42 PM boaz0 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In webpack/assets/javascripts/react_app/components/ModelsTable/ModelsTable.js <#5755 (comment)>: > @@ -0,0 +1,57 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import { Table } from '../common/table'; +import { STATUS } from '../../constants'; +import MessageBox from '../common/MessageBox'; +import { translate as __ } from '../../common/I18n'; +import createModelsTableSchema from './ModelsTableSchema'; +import { getURIQuery } from '../../common/helpers'; + +class ModelsTable extends React.Component { + componentDidMount() { + this.props.getTableItems('models', getURIQuery(window.location.href)); Need a follow up: Pagination to be rendered inside the ModelsTable. Right now it is rendered from erb. I am checking how much work it required. If it is not a lot, I will update this commit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5755 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOx9YpEzi18CWv4m6f4tWm4ChpltRfks5vNVnIgaJpZM4U8um9> .

ohadlevy · 2019-02-14T17:53:24Z

@glekner can you review please? thanks.

I don't have time to check again.

timogoebel · 2019-02-15T08:08:46Z

@ohadlevy: I trust you with this one. Feel free to merge whenever you think this is ready.

sharvit

LGTM

sharvit · 2019-02-17T07:49:16Z

Test failures seem related

glekner

Awesome work @boaz0 !
I believe we need to change render method of the table

glekner · 2019-02-17T09:54:25Z

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

+ * the parameters and the values are the parameters' values.
+ * @param {String} url - the URL
+ */
+export const getURIQuery = url => new URI(url).query(true);


We'll probably want to move all the URI helpers from PaginationHelper to common/helpers or another file in the future.

glekner · 2019-02-17T10:07:01Z

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

+      );
+
+    return (
+      <Spinner size="lg" loading={this.state.initialLoading}>


I have mixed feelings about this.
I think its good to show a spinner for the initial loading, but it also shows when changing pages,
the page flickers and the table disappears.

IMO, you should check the state, if the results array is empty, show a spinner, if not show the table. this way, there won't be any flickers, and I think you can remove the internal state of the component along with the componentDidUpdate and stopSpinner

Suggested change

<Spinner size="lg" loading={this.state.initialLoading}>

if (results.length > 0) return renderTable;

return <Spinner size="lg" loading />;

ohadlevy · 2019-02-17T11:10:18Z

can't we reuse https://github.com/patternfly/patternfly-react/pull/886/files ?

…

On Sun, Feb 17, 2019 at 12:09 PM Gilad Lekner ***@***.***> wrote: ***@***.**** requested changes on this pull request. Awesome work @boaz0 <https://github.com/boaz0> ! I believe we need to change render method of the table ------------------------------ In webpack/assets/javascripts/react_app/common/helpers.js <#5755 (comment)>: > @@ -72,6 +73,13 @@ export const translateObject = obj => */ export const translateArray = arr => arr.map(str => __(str)); +/** + * Return the query in URL as Objects where keys are + * the parameters and the values are the parameters' values. + * @param {String} url - the URL + */ +export const getURIQuery = url => new URI(url).query(true); We'll probably want to move all the URI helpers from PaginationHelper to common/helpers or another file in the future. ------------------------------ In webpack/assets/javascripts/react_app/components/ModelsTable/ModelsTable.js <#5755 (comment)>: > + /> + ) : ( + <React.Fragment> + <Table + key="models-table" + columns={createModelsTableSchema(getTableItems, sortBy, sortOrder)} + rows={results} + /> + <div id="pagination"> + <Pagination data={pagination} /> + </div> + </React.Fragment> + ); + + return ( + <Spinner size="lg" loading={this.state.initialLoading}> I have mixed feelings about this. I think its good to show a spinner for the initial loading, but it also shows when changing pages, the page flickers and the table disappears. IMO, you should check the state, if the results array is empty, show a spinner, if not show the table. this way, there won't be any flickers, and I think you can remove the internal state of the component along with the componentDidUpdate and stopSpinner ⬇️ Suggested change - <Spinner size="lg" loading={this.state.initialLoading}> + if (results.length > 0) return renderTable; + return <Spinner size="lg" loading />; — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5755 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOxw4qoDx93qx0bLSBNcqG7cq1Rd8-ks5vOSpzgaJpZM4U8um9> .

ohadlevy · 2019-02-20T12:49:54Z

@boaz0 what did you decided to do with the PR? please let me know AFAIU the options are:

revert back to ERB generating 3 react components (search, table, pagination)
combine the table and pagination into one component?

thanks!

iNecas

Seems like Ohad's request for reusing the loading state was incorporated. Tested from user's perspective and works well.

I've hit some weird stuff when having Redux dev tools enabled (I guess it might be related to the fact we're mixing redux with turbolinks or something like that) but haven't observed this weirdness wiwhout the Redux dev tools.

Rebase needed (+ probably fixing some pending tests)

ohadlevy · 2019-02-21T09:22:03Z

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

+  );
+};
+
+TableBody.propTypes = {


I'm not sure where this is coming from (pf?) but I get this error in my console:

checkPropTypes.js:19 Warning: Failed prop type: The prop `children` is marked as required in `EllipisWithTooltip`, but its value is `null`. in EllipisWithTooltip (created by BodyRow) in BodyRow (created by Body) in tbody (created by Body) in Body (created by TableBody) in TableBody (created by Table) in table (created by Provider) in Provider (created by TablePfProvider) in TablePfProvider (created by Table) in div (created by Table) in Table (created by ModelsTable) in LoadingState (created by ModelsTable) in ModelsTable (created by Connect(ModelsTable)) in Connect(ModelsTable) (created by I18nProviderWrapper(Connect(ModelsTable))) in IntlProvider (created by I18nProviderWrapper(Connect(ModelsTable))) in I18nProviderWrapper(Connect(ModelsTable)) (created by StoreProvider(I18nProviderWrapper(Connect(ModelsTable)))) in Provider (created by StoreProvider(I18nProviderWrapper(Connect(ModelsTable)))) in StoreProvider(I18nProviderWrapper(Connect(ModelsTable))) (created by DataProvider(StoreProvider(I18nProviderWrapper(Connect(ModelsTable))))) in DataProvider(StoreProvider(I18nProviderWrapper(Connect(ModelsTable))))

I couldn't reproduce it. I guess it's because I am not checking the value in common/table/formatters/ellipsisCellFormatter.js. Let me know if it is fixed.

ohadlevy · 2019-02-21T09:26:13Z

@amirfefer @sharvit @LaViro I've rebased this PR, can you please have another look prior to merge? I'm OK with small follow up PR's if you think that is required, but I think this PR is pretty much ready? thanks!

Ron-Lavi

Awesome work @boaz0 !!
Left some inline comments which I am fine with adding to a follow up PR :)

Ron-Lavi · 2019-02-21T09:45:11Z

app/views/models/index.html.erb

-</table>
-<%= will_paginate_with_info @models %>
+<div id="models_table"></div>
+<%= mount_react_component("ModelsTable", "#models_table", {


could be added as a follow up PR,
Maybe create an helper file for it, e.g.: search_bar_helper.rb
and maybe use the component wrapper so props won't be accessed in react by the data object.

IMO we should use the component-wrapper only when we want to update those props with redux.
data prop became a standard for us to know those props came from server rendering and it feels useful to me.

@amirfefer what are you thoughts ?
can you share about the flat option in mount_react_component as we discussed offline ?

you can use {flattenData: true} in the componentRegistery,
I do agree with @sharvit on this, but if we use a component both server and client rendering (like charts) IMO it would be better to use the flattenData

Ron-Lavi · 2019-02-21T10:00:45Z

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

+
+class ModelsTable extends React.Component {
+  componentDidMount() {
+    this.props.getTableItems('models', getURIQuery(window.location.href));


we started to use more objects destruction in our components:
e.g: const { getTableItems } = this.props

if you want to get the query from the url, you could use the URI library which is already used in the project,
https://medialize.github.io/URI.js/docs.html#accessors-search

Ron-Lavi · 2019-02-21T10:01:17Z

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

+      error,
+      status,
+      results,
+      data: { pagination },


as mentioned above, if we use the component wrapper, we won't need the data object here.

boaz0 · 2019-02-21T10:15:49Z

@ohadlevy I will revert models.html.erb and @glekner will take it from there.

boaz0 · 2019-02-21T10:18:08Z

@LaViro feel free to open tickets and work on those. I think this PR is old and bloated.

sharvit

LGTM! Thanks @boaz0 🥇

ohadlevy · 2019-02-21T14:32:40Z

how is this different than other places we use the mounter? if its not specific to this PR, lets do this in a follow up pr for all mount points?

…

On Thu, Feb 21, 2019 at 4:14 PM Avi Sharvit ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/views/models/index.html.erb <#5755 (comment)>: > - hash_for_edit_model_path(:id => model).merge(:auth_object => model, :authorizer => authorizer) %></td> - <td class="ellipsis"><%= model.vendor_class %></td> - <td class="ellipsis"><%= model.hardware_model %></td> - <td class='ra'><%= link_to hosts_count[model], hosts_path(:search => "model = \"#{model}\"") %></td> - <td class="col-md-1"> - <%= action_buttons(display_delete_if_authorized(hash_for_model_path(:id => model). - merge(:auth_object => model, :authorizer => authorizer), - :confirm => _("Delete %s?") % model.name)) %> - </td> - </tr> - <% end %> - </tbody> -</table> -<%= will_paginate_with_info @models %> +<div id="models_table"></div> +<%= mount_react_component("ModelsTable", "#models_table", { IMO we should use the component-wrapper only when we want to update those props with redux. data prop became a standard for us to know those props came from server rendering and it feels useful to me. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5755 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOx28izb1_tck4uOm2OdXZjFTKlGAnks5vPqnCgaJpZM4U8um9> .

ohadlevy · 2019-02-21T15:55:16Z

test failures not related, thanks!

sharvit · 2019-02-21T15:57:51Z

Thanks @boaz0 and everyone who contributed to this PR ❤️

theforeman-bot added Needs testing Not yet reviewed labels Jun 29, 2018

houndci-bot reviewed Jun 29, 2018

View reviewed changes

app/models/model.rb Outdated Show resolved Hide resolved

app/models/model.rb Outdated Show resolved Hide resolved

app/models/model.rb Outdated Show resolved Hide resolved

app/models/model.rb Outdated Show resolved Hide resolved

app/models/model.rb Outdated Show resolved Hide resolved

timogoebel reviewed Jun 29, 2018

View reviewed changes

app/models/model.rb Outdated Show resolved Hide resolved

waldenraines reviewed Jun 29, 2018

View reviewed changes

houndci-bot reviewed Jun 29, 2018

View reviewed changes

app/models/model.rb Outdated Show resolved Hide resolved

ohadlevy reviewed Jul 1, 2018

View reviewed changes

app/controllers/models_controller.rb Outdated Show resolved Hide resolved

app/controllers/models_controller.rb Outdated Show resolved Hide resolved

app/models/model.rb Outdated Show resolved Hide resolved

ohadlevy suggested changes Jul 1, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jul 1, 2018

ohadlevy reviewed Jul 1, 2018

View reviewed changes

app/views/models/index.html.erb Outdated Show resolved Hide resolved

ohadlevy reviewed Jul 1, 2018

View reviewed changes

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

boaz0 mentioned this pull request Jul 3, 2018

Fixes #21737 - move about page to react #5024

Closed

4 tasks

amirfefer suggested changes Jul 4, 2018

View reviewed changes

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

houndci-bot reviewed Jul 4, 2018

View reviewed changes

app/controllers/models_controller.rb Outdated Show resolved Hide resolved

houndci-bot reviewed Jul 4, 2018

View reviewed changes

app/controllers/models_controller.rb Outdated Show resolved Hide resolved

houndci-bot reviewed Jul 4, 2018

View reviewed changes

app/controllers/models_controller.rb Outdated Show resolved Hide resolved

app/controllers/models_controller.rb Outdated Show resolved Hide resolved

ohadlevy suggested changes Jul 5, 2018

View reviewed changes

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

sharvit previously approved these changes Feb 17, 2019

View reviewed changes

glekner suggested changes Feb 17, 2019

View reviewed changes

theforeman-bot added the Waiting on contributor label Feb 17, 2019

boaz0 dismissed sharvit’s stale review via bb015d2 February 17, 2019 13:43

theforeman-bot added Needs re-review and removed Waiting on contributor labels Feb 17, 2019

amirfefer mentioned this pull request Feb 19, 2019

[WIP] Fixes #26078 - interface table in react #6490

Closed

iNecas previously approved these changes Feb 20, 2019

View reviewed changes

theforeman-bot removed the Needs re-review label Feb 20, 2019

ohadlevy dismissed iNecas’s stale review via d33582f February 21, 2019 09:09

ohadlevy reviewed Feb 21, 2019

View reviewed changes

Ron-Lavi reviewed Feb 21, 2019

View reviewed changes

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

3582e14

sharvit approved these changes Feb 21, 2019

View reviewed changes

ohadlevy merged commit 35e37e3 into theforeman:develop Feb 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

boaz0 commented Jun 29, 2018 •

edited

Loading

theforeman-bot commented Jun 29, 2018

waldenraines left a comment

ohadlevy left a comment

ohadlevy Jul 1, 2018

amirfefer left a comment •

edited

Loading

ohadlevy commented Feb 13, 2019

boaz0 commented Feb 14, 2019

ohadlevy commented Feb 14, 2019 via email

ohadlevy commented Feb 14, 2019

timogoebel commented Feb 15, 2019

sharvit left a comment

sharvit commented Feb 17, 2019

glekner left a comment

glekner Feb 17, 2019

glekner Feb 17, 2019

ohadlevy commented Feb 17, 2019 via email

ohadlevy commented Feb 20, 2019

iNecas left a comment

ohadlevy Feb 21, 2019

boaz0 Feb 21, 2019

ohadlevy commented Feb 21, 2019

Ron-Lavi left a comment

Ron-Lavi Feb 21, 2019

sharvit Feb 21, 2019

Ron-Lavi Feb 21, 2019

amirfefer Feb 21, 2019

Ron-Lavi Feb 21, 2019

Ron-Lavi Feb 21, 2019

boaz0 commented Feb 21, 2019

boaz0 commented Feb 21, 2019

sharvit left a comment

ohadlevy commented Feb 21, 2019 via email

ohadlevy commented Feb 21, 2019

sharvit commented Feb 21, 2019

	<Spinner size="lg" loading={this.state.initialLoading}>
	if (results.length > 0) return renderTable;
	return <Spinner size="lg" loading />;

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

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

Conversation

boaz0 commented Jun 29, 2018 • edited Loading

theforeman-bot commented Jun 29, 2018

waldenraines left a comment

Choose a reason for hiding this comment

ohadlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer left a comment • edited Loading

Choose a reason for hiding this comment

ohadlevy commented Feb 13, 2019

boaz0 commented Feb 14, 2019

ohadlevy commented Feb 14, 2019 via email

ohadlevy commented Feb 14, 2019

timogoebel commented Feb 15, 2019

sharvit left a comment

Choose a reason for hiding this comment

sharvit commented Feb 17, 2019

glekner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy commented Feb 17, 2019 via email

ohadlevy commented Feb 20, 2019

iNecas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy commented Feb 21, 2019

Ron-Lavi 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

boaz0 commented Feb 21, 2019

boaz0 commented Feb 21, 2019

sharvit left a comment

Choose a reason for hiding this comment

ohadlevy commented Feb 21, 2019 via email

ohadlevy commented Feb 21, 2019

sharvit commented Feb 21, 2019

boaz0 commented Jun 29, 2018 •

edited

Loading

amirfefer left a comment •

edited

Loading