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

reactjs pattern library components #91

Merged
merged 4 commits into from
May 21, 2018
Merged

reactjs pattern library components #91

merged 4 commits into from
May 21, 2018

Conversation

dankins
Copy link
Member

@dankins dankins commented Apr 16, 2018

No description provided.

@@ -0,0 +1 @@
/dist/
Copy link
Contributor

Choose a reason for hiding this comment

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

how about standardising with the rest of the packages and using build/ for builds

README.md Outdated
@@ -17,6 +17,7 @@ We hope that those tools will be useful for creation of interesting applications
| [`@joincivil/core`][core-url] | [![npm link](https://img.shields.io/badge/npm-core-blue.svg)](https://www.npmjs.com/package/@joincivil/core) | JS library for interacting with Civil ecosystem |
| [`@joincivil/tslint-rules`](/packages/tslint-rules) | [![npm link](https://img.shields.io/badge/npm-tslint--rules-blue.svg)](https://www.npmjs.com/package/@joincivil/tslint-rules) | Linting rules for Civil's Typescript packages |
| [`@joincivil/utils`](/packages/utils) | [![npm link](https://img.shields.io/badge/npm-utils-blue.svg)](https://www.npmjs.com/package/@joincivil/utils) | Utilities shared between Civil projects used during runtime |
| [`@joincivil/components`](/packages/components) | [![npm link](https://img.shields.io/badge/npm-components-blue.svg)](https://www.npmjs.com/package/@joincivil/components) | ReactJS components used in Civil webapps |
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference between the components from dapp? Move them from dapp, or merge this package with dapp?

Copy link
Member Author

Choose a reason for hiding this comment

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

dapp could use components from this library. mostly this is for civil-web and editor

"build": "tsc",
"lint": "tslint --exclude \"**/storyFixtures/**\" --project ./",
"clean": "rimraf build/",
"prepublish": "yarn build",
Copy link
Contributor

Choose a reason for hiding this comment

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

Use npm-run-all package and do run-s build to make it work with all managers and systems


import { colors, fonts } from "./styleConstants";

export interface ButtonProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

are any of those required?
If so you can use:

interface ButtonProps {
   className: string;
   ...
}

interface ButtonOptions extends Partial<ButtonProps> {
  onClick?(ev: any): any;
}

disabled?: boolean;
inputRef?: any;
name?: string;
onClick?(ev: any): any;
Copy link
Contributor

Choose a reason for hiding this comment

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

if the result is not important you can use onClick?(ev: any): void and it will work

background-color: ${colors.accent.CIVIL_BLUE_FADED};
color: ${colors.basic.WHITE};
}
:disabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

&:disabled?

constructor(props: FullScreenModalProps) {
super(props);
this.state = {};
if (typeof document !== "undefined") {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not document !== undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

if document is not defined this would throw an error "document is not defined", whereas if you do typeof document it returns the string "undefined"

CIVIL_BLUE_1: "#2B56FF",
},
accent: {
CIVIL_YELLOW: "#FF120", // Civil-wide accent color
Copy link
Contributor

Choose a reason for hiding this comment

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

I would enjoy more descriptive names telling what scenarios are they used in

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied these from the styled guide. feel free to color (heh) this in a bit more @juliahimmel !

@ritave
Copy link
Contributor

ritave commented Apr 16, 2018

npm-run-all is the only important thing here

@nickreynolds
Copy link
Member

what's the status of this PR?

@dankins
Copy link
Member Author

dankins commented Apr 24, 2018

waiting for @walfly to review

@coveralls
Copy link

coveralls commented May 21, 2018

Pull Request Test Coverage Report for Build 3243

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 87.539%

Totals Coverage Status
Change from base Build 3236: 0.0%
Covered Lines: 581
Relevant Lines: 625

💛 - Coveralls

@walfly walfly merged commit a16d30a into master May 21, 2018
@ritave ritave deleted the dankins/components branch June 20, 2018 11:01
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.

None yet

5 participants