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

Add refactored table with search and sort function #27

Merged
merged 16 commits into from
Apr 6, 2021

Conversation

mamy-CS
Copy link
Collaborator

@mamy-CS mamy-CS commented Mar 26, 2021

@mamy-CS mamy-CS self-assigned this Mar 26, 2021
@mamy-CS mamy-CS linked an issue Mar 26, 2021 that may be closed by this pull request
Copy link
Owner

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

Love the tables! Really cool! Have a couple nits and some sync issues to clarify...

tornjak-frontend/src/components/agent-list.component.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/agent-list.component.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/agent-list.component.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/entry-list.component.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/entry-list.component.js Outdated Show resolved Hide resolved
tornjak-frontend/src/tables/agentsListTable.js Outdated Show resolved Hide resolved
tornjak-frontend/src/tables/agentsListTable.js Outdated Show resolved Hide resolved
tornjak-frontend/src/tables/agentsListTable.js Outdated Show resolved Hide resolved
tornjak-frontend/src/tables/agentsListTable.js Outdated Show resolved Hide resolved
Copy link
Owner

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

Besides the synchronization loop issue, LGTM.

tornjak-frontend/src/tables/entriesListTable.js Outdated Show resolved Hide resolved
tornjak-frontend/src/tables/agentsListTable.js Outdated Show resolved Hide resolved
tornjak-frontend/src/tables/entriesListTable.js Outdated Show resolved Hide resolved
window.location.reload();
i = 0;
Promise.all(promises)
.then(responses => responses.forEach(response => {
Copy link
Owner

Choose a reason for hiding this comment

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

forEach here still creates sync issues because each element isnt independent in its processing. a regular for loop should work here.

this.props.agentsListUpdate(this.props.globalagentsList.filter(el =>
el.id.trust_domain !== id[i].trust_domain ||
el.id.path !== id[i].path));
i++;
Copy link
Owner

Choose a reason for hiding this comment

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

double i++ increment.

@lumjjb
Copy link
Owner

lumjjb commented Apr 6, 2021

Build error:

src/tables/agentsListTable.js
  Line 116:109:  'prefix' is not defined  no-undef

Copy link
Owner

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

LGTM! Looks great!

@lumjjb lumjjb merged commit f9f9eb8 into lumjjb:main Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UI+logic]Refactor Tables with sort and filter
2 participants