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

Fixes #27272 - ajax load for table data from dashboard actions #432

Merged
merged 1 commit into from Jul 12, 2019

Conversation

iNecas
Copy link
Member

@iNecas iNecas commented Jul 11, 2019

No description provided.

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

The user experience is now much better, ACK

<th class="col-md-2"><%= sort :ended_at, :as => _("Ended at") %></th>
<th class="col-md-2"><%= _("User") %></th>
</tr>
<% for task in @tasks %>
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL there's for item in collection syntax in ruby

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

8-O (applies to both comments)

$('#tasks-table').html(res.find('#tasks-table'));
},
error({ statusText }) {
$('#tasks-table').html(statusText);
Copy link
Member

Choose a reason for hiding this comment

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

should this hide spinner too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that handled by the complete callback?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it is

@adamruzicka
Copy link
Contributor

Needs a rebase

@iNecas
Copy link
Member Author

iNecas commented Jul 12, 2019

Rebased

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Works well, ACK

@adamruzicka adamruzicka merged commit 0b6bdc6 into theforeman:master Jul 12, 2019
@adamruzicka
Copy link
Contributor

Thanks @iNecas !

Copy link
Contributor

@sharvit sharvit left a comment

Choose a reason for hiding this comment

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

Thanks to everyone who contributed to this PR.

We are planning for moving the table to react and use react-router to update this table.
AFAIK @MariaAga is already working on it and have great progress.

Notice the resolveQuery designed to be a workaround and to be removed once we implement the new table with react.

So is this PR need to be reverted eventually?
Should it be removed as part of @MariaAga work?

I feel like @MariaAga had to be involved in this discussion.

@@ -73,6 +73,23 @@ export const resolveQuery = query => {
const { search } = uri.query(true);

const data = { search, ...uriQuery, page: 1 };
const { $, tfm } = window;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using variables from the window object considre a bad practice in webpack.
Better doing import $ from 'jquery';

About the tfm, it designed to be the bridge between webpack and the assets pipeline so we can slowly replace old code in the assets gradually. It considers wrong to use this object inside webpack.

uri.query(URI.buildQuery(data, true));
window.location.href = uri.toString();
tfm.tools.showSpinner();
Copy link
Contributor

Choose a reason for hiding this comment

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

It considered wrong to use the showSpinner from the tfm object. Instead, something like this should be done in the action:

import { showSpinner, hideSpinner } from 'foremanReact/components/Layout/LayoutActions';

// inside an action
const myAction = () => async (dispatch) => {
  try {
    dispatch(showSpinner());
    disptach(requestStarted());

    const results = await API.get(...);
    dispatch(requestSucceed(results));
  } catch (error) {
    dispatch(requestError(error));
  }

    dispatch(hideSpinner());
};

uri.query(URI.buildQuery(data, true));
window.location.href = uri.toString();
tfm.tools.showSpinner();
$.ajax({
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the ajax function from jquery considre wrong, in webpack we prefer to use the shared: https://github.com/theforeman/foreman/blob/ac1fe0de56f29d834a54a96d069231a823878ed3/webpack/assets/javascripts/react_app/API.js

  • It might not work for this case because the API designed to receive jsons.

@iNecas
Copy link
Member Author

iNecas commented Jul 18, 2019

@sharvit exactly: this is purely to provide a short-term workaround for the ugly behavior we had there before and I'm all for removing this once @MariaAga's work is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants