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

Migrate components (1st batch) #4

Merged
merged 17 commits into from
Mar 17, 2017
Merged

Conversation

zhusee2
Copy link
Contributor

@zhusee2 zhusee2 commented Mar 15, 2017

Purpose

Migrate components from iC-framework-react.

Components

📦 <Tag>
📦 <Icon>
📦 <TextEllipsis>
📦 <StatusIcon> (slightly modified)
📦 <FlexCell> (slightly modified)

Other implements

  1. Config Webpack to handle font files with file-loader
  2. Update simple documents.

Demo

2017-03-15 4 49 09

@zhusee2 zhusee2 requested a review from cjies March 15, 2017 09:19
Copy link
Contributor

@cjies cjies left a comment

Choose a reason for hiding this comment

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

The .scss files should placed in src/sass or src/scss folder? 🤔

doc/DocApp.js Outdated
@@ -1,8 +1,14 @@
import React from 'react';
import App from '../src';
import '../src/css/index.scss';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add src and doc as the aliases in webpack to resolve their paths.

webpackBaseConfig = {
    resolve: {
        alias: {
            src: path.resolve(__dirname, '../src'),
            doc: path.resolve(__dirname, '../doc')
        }
    }
}

@@ -0,0 +1,208 @@
@import "./animations";
Copy link
Contributor

@cjies cjies Mar 15, 2017

Choose a reason for hiding this comment

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

Just take a note here.

If we import the resources that will be actually rendered in CSS, it will be added to every imported SASS file. Maybe we can try cssnano to discard the duplicated parts.

But this suggestion is not the main scope of this PR, I will open a PR to fix this problem later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or I can simply move them to index.scss?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's is a good idea 👍

@zhusee2
Copy link
Contributor Author

zhusee2 commented Mar 16, 2017

Regarding the naming of stylesheets folder:

I think I'll just pick styles/ so it can hold all kinds of stylesheets (though we're pretty much only going to use Sass.)

@zhusee2 zhusee2 force-pushed the feature/zhusee_migrate_1st_batch branch from affc35f to b557b5e Compare March 16, 2017 06:24
@zhusee2
Copy link
Contributor Author

zhusee2 commented Mar 16, 2017

Post-review changes

  1. Use babel-plugin-module-resolver for module aliases.
    I choose to set alias on Babel because almost every JS file will go through Babel before they reach other tools such as Webpack or Jest.
    It's easier to maintain aliases in one single place by asking Babel to transform the import path first.

  2. Rename /src/css to /src/styles
    So it can possibly hold other stylesheets in the future.

  3. **Move @import "./animations" to index.scss
    So it won't duplicate if more than one file imports it.

@zhusee2 zhusee2 merged commit 920c6ca into develop Mar 17, 2017
@zhusee2 zhusee2 deleted the feature/zhusee_migrate_1st_batch branch March 17, 2017 05:59
@zhusee2 zhusee2 mentioned this pull request May 24, 2017
18 tasks
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

2 participants