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

Updated ESLint version & rules, then fix code across project, and fixed #212 #213

Merged
merged 8 commits into from
Mar 13, 2017

Conversation

haxpor
Copy link
Contributor

@haxpor haxpor commented Mar 11, 2017

Updated ESLint version. and its rules and fixed warnings/errors across project. Also with fixing of #212

  • already tested via npm run test and all tests passed.
  • most of code fixed across the project are about semi colon, space
    after if/else if/else/for statement, removed newline at the end of file, a few dangerous code statement fixed, and a few that is not needed.
  • ESlint is updated to version 3.17.1. The project has used version
    1.x.x. This should be better.
  • ESLint's rules are updated to be more stricted but not too strict. See
    it in .eslintrc.
  • fixed Grid should also work properly with URL as set from react-router #212

Fixed #212

Tested, It works fine and can provide normal <a> functionality to link to normal URL, or react-router's URL.

We can use the following code in Grid's Data.

const data = [
  {
    icon: <img src={issue_icon} alt="issue icon"/>,
    label: 'Issues ...',
    to: '/issues'
  },
  {
    icon: <img src={project_icon} alt="project icon"/>,
    label: 'Projects',
    to: '/projects'
  },
  {
    icon: <img src={contribute_icon} alt="contribute icon"/>,
    label: 'Contributes',
    href: 'https://google.com'
  }
];

The first two items are for react-router's URL, and the third one is for normal URL. When click, it takes to destination properly.

Anyway, the code seems easy and straightforward but not sure it should be the approach use or not as it directly use react-router's <Link> and I checked react-weui already depends on it although older version.

The reason is that starting from version 1.0 of ESLint, all linting
rules are turned off by default.
- already tested via `npm run test` and all tests passed.
- most of code fixed across the project are about semi colon, space
  after if/else if/else/for statement, removed newline at the end of file, a few dangerous code statement fixed, and a few that is not needed.
- ESlint is updated to version 3.17.1. The project has used version
  1.x.x. This should be better.
- ESLint's rules are updated to be more stricted but not too strict. See
  it in .eslintrc.
@haxpor haxpor changed the title Updated ESLint version, its rules and fixed code to adhere rules across the project Updated ESLint version & rules, fixed code across project for updated eslint, and fixed #212 Mar 12, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-6.5%) to 82.097% when pulling a5757f2 on haxpor:master into ace7de6 on weui:master.

@haxpor haxpor mentioned this pull request Mar 12, 2017
@haxpor haxpor changed the title Updated ESLint version & rules, fixed code across project for updated eslint, and fixed #212 Updated ESLint version & rules, then fix code across project, and fixed #212 Mar 12, 2017
@n7best n7best self-requested a review March 12, 2017 15:25
@@ -2,6 +2,7 @@ import React from 'react';
import classNames from 'classnames';
import GridIcon from './grid_icon';
import GridLabel from './grid_label';
import { Link } from 'react-router';
Copy link
Collaborator

Choose a reason for hiding this comment

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

@haxpor Thanks for the PR and work to the lint!

I believe to use Link in this case will require dependency from react-router and not everyone use react-router as router even though most of the single page application do.

Can you modify it to something like Cell that people will be able to pass into their component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree! That's much more robust.

I've fixed it to provide component property to be set in Grid as similar to Cell class.

- provided 'component' property to be able to replace component instead
  of using react-router, more flexible

  example of setting from Grid's data from code to use react-router url is as follows

  const data = [
  {
    icon: <img src="./image.png" alt="image icon"/>,
    label: 'Label',
    component: function(props) {
        const { children, ...others } = props;
        return <Link to="/router-url" {...others}> { children } </Link>;
    }
  },
  ]
@coveralls
Copy link

Coverage Status

Coverage decreased (-6.5%) to 82.068% when pulling ec45e68 on haxpor:master into ace7de6 on weui:master.

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.

Grid should also work properly with URL as set from react-router
3 participants