Skip to content

Client react version#76

Open
Shannibrst wants to merge 6 commits into
masterfrom
client-react-version
Open

Client react version#76
Shannibrst wants to merge 6 commits into
masterfrom
client-react-version

Conversation

@Shannibrst

Copy link
Copy Markdown
Collaborator

Please review code and merge if everything looks ok.

@art4ride art4ride left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Commented out a number of issues. Will test it on my local after they'll be fixed

Comment thread client/react/actions/clientActions.js Outdated
@@ -0,0 +1,7 @@
import { Clients } from '../../../imports/api/clients/clients';

export const getClientProfile = function getClientProfile(clientId) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like this is not in use. What's the purpose of that code? If you need it for testing please move it to /imports/__tests__/fixtures

display: block !important;
}
.custom .show {
display: none !important;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

show class with display: none seems confusing

Comment thread client/react/component/ClientHistory.js Outdated
import { Clients } from '../../../imports/api/clients/clients';


const reasons = [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you please move reasons to a separate file. Otherwise it will be hard to find

Comment thread client/react/component/ClientHistory.js Outdated
});
const clientIds = clientVersions.map(({ clientId }) => clientId);

const remarks = $('#removalRemarks').val();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please use react way of getting values instead of jQuery one?

Comment thread client/react/component/ClientHistory.js Outdated
const clientVersions = client.clientVersions
.filter(({ clientId, schema }) => {
const data = this.getEligibleClient(clientId, schema);
if(!!(data)){

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

!! is not needed

Comment thread client/react/react.css
.tab-section .nav-tabs.nav-justified > li.active > a:hover {
border: none;
}
.css-10nd86i {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please use semantic names for styling

Comment thread client/react/react.css
position: relative;
box-sizing: border-box;
}
.css-vj8t7z {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above

Comment thread client/react/react.html
@@ -0,0 +1,8 @@
<head>
<title>Home-App</title>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this in use? We're not switching to React completely yet

Comment thread client/react/react.js Outdated
format: 'MM-DD-YYYY',
});

$('body').on('click', '.tag_form_cancel', function () {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you please handle those either in React or Blaze/Meteor way?

Comment thread client/react/routes.js

export const renderRoutes = () => (
<Router>
<Route path="/clientr/:clientId" component={ClientItem} />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clientr ?
Ans we're not using react-router at the moment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants