Skip to content
This repository was archived by the owner on Apr 22, 2025. It is now read-only.

Fixed issue#870#871

Closed
Katy310 wants to merge 4 commits intoopenfarmcc:masterfrom
Katy310:issue#870
Closed

Fixed issue#870#871
Katy310 wants to merge 4 commits intoopenfarmcc:masterfrom
Katy310:issue#870

Conversation

@Katy310
Copy link
Copy Markdown
Contributor

@Katy310 Katy310 commented Dec 12, 2016

Removed the red border.

Removed the red border.
border: 1px solid $alert-color;
}

input#search_crop_name.ng-invalid {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid using id selectors
Avoid qualifying id selectors with an element.
Selector search_crop_name should be written in lowercase with hyphens

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Katy310. I think this is a fair comment. I know that I suggested the above selector, but maybe there's a better way to do so? Do you have time to make a fix to your PR?

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 99.352% when pulling 99833a8 on Katy310:issue#870 into c325531 on openfarmcc:master.

border: 1px solid $alert-color;
}

input#search_crop_name.ng-invalid {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid using id selectors
Avoid qualifying id selectors with an element.
Selector search_crop_name should be written in lowercase with hyphens

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is still the same issue from before the update :).

input.ng-invalid {
input-invalid-notify{
border: none;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rule declaration should be followed by an empty line


input.ng-invalid {
input-invalid-notify{
border: none;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Line should be indented with spaces, not tabs

}

input.ng-invalid {
input-invalid-notify{
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Opening curly brace { should be preceded by one space

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 99.352% when pulling fa63c72 on Katy310:issue#870 into c325531 on openfarmcc:master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 99.352% when pulling 1356f86 on Katy310:issue#870 into c325531 on openfarmcc:master.

Copy link
Copy Markdown
Member

@simonv3 simonv3 left a comment

Choose a reason for hiding this comment

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

Hey @Katy310, thanks for the PR! I made some inline comments. Overall - looks like maybe some other files weren't included in the commit?

border: none;
}

.input-invalid-notify.ng-invalid {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this class exist in the HTML? Maybe you forgot to add it to the commit?

}

input.ng-invalid {
input-invalid-notify {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you adding a directive called input-invalid-notify? If so, is there a new directive file that should be part of this commit?

border: 1px solid $alert-color;
}

input#search_crop_name.ng-invalid {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is still the same issue from before the update :).

@simonv3 simonv3 closed this Feb 17, 2017
@simonv3
Copy link
Copy Markdown
Member

simonv3 commented Feb 17, 2017

This was fixed in #885

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants