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 table, Support multiply boards and slide them periodly #11

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

KongHuihui-CAD
Copy link

No description provided.

Copy link
Contributor

@huan huan left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I have some reviews that need you to follow, please let me know if you have any questions.

And please remember to reference the issue number (like #10 for this one) in the description of your Pull Request.

import { ElModule } from 'element-angular'

// if you use webpack, import style
import 'element-angular/theme/index.css'
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we only include the css in the component files?

Because I do not think include css into app.module.ts is a good idea.

Copy link
Author

Choose a reason for hiding this comment

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

ok,I will amend as soon as possible

title: string,
type: string,
data: Array<Array<string | number | {}>>,
roles: Array<{ type: string, role: string }>,
Copy link
Contributor

Choose a reason for hiding this comment

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

image

Please keep the original format: vertical align the :, thanks.

Copy link
Contributor

@huan huan left a comment

Choose a reason for hiding this comment

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

Thanks for following my reviews.

I'd like to merge your PR after you make the CI(Continous Integration) green by making sure your new code can be able to pass all the automatic tests.

Current it had failed: see the result at https://travis-ci.com/BUPT/cad-board/builds/91834721

You can reproduce the test result by run npm test locally.

BTW: Please also add the issue number to track the issue at the description of this PR.

@huan
Copy link
Contributor

huan commented Nov 19, 2018

Great to see your update because it seems that you had fixed the linting issues.

However, there's still some problem with the angular unit tests, please fix them as well.

@linyimin0812
Copy link
Member

I think the test case should be modified, When we use setInternal () to support multiply boards and sliding them periodly, the test case will say timeout,and test case can't pass. Could you give me some advice?Thanks.

@huan
Copy link
Contributor

huan commented Nov 26, 2018

@linyimin-bupt It seems that you had run into issues #1

Please also have a look at: angular/protractor#4300 (comment) and angular/angularfire#779

Hope those informations will be helpful.

Add repeat function
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.

3 participants