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

Attempt to revise <TextLabel> (pt. 2) #7

Merged
merged 27 commits into from
Mar 27, 2017
Merged

Conversation

zhusee2
Copy link
Contributor

@zhusee2 zhusee2 commented Mar 23, 2017

Purpose

Finish the revised-and-simplified <TextLabel> and its foundations that will be using to build all other row components.

Concept

The components that will be placed inside a row-container is called Row Component.
Therefore the mixin for them is named rowComp().

A RowComp can be used in the following 2 ways:

Use pre-configured layout by passing everything via props

<TextLabel
    basic="Basic Text"
    tag="Tag"
    aside="Aside Text"
    align="center"
    status"loading" />

Customize layout via children

<TextLabel status="error">
    <Icon type="star" />
    <Text basic="Announcements" />
    <Icon type="star" />
</TextLabel>

Implement

  1. Add rowComp() and withStatus() HOC mixins.
  2. <Text> is now pre-wrapped with withStatus() to auto handle status from context.
  3. Add <TextLabel>, which is a plain component wrapped with rowComp()
  4. Update state styles for row components.
  5. Bump version to 0.4.0

Package dependencies

babel-runtime is now listed as a peer dependency, instead of a dev dependency.
When you take a look at the top of the Babel-transformed StatusIcon.js, you can see:

import _Object$values from 'babel-runtime/core-js/object/values';
import _Object$getPrototypeOf from 'babel-runtime/core-js/object/get-prototype-of';
import _classCallCheck from 'babel-runtime/helpers/classCallCheck';
import _createClass from 'babel-runtime/helpers/createClass';
import _possibleConstructorReturn from 'babel-runtime/helpers/possibleConstructorReturn';
import _inherits from 'babel-runtime/helpers/inherits';
import React, { PureComponent, PropTypes } from 'react';
import icBEM from './utils/icBEM';

It's obvious that the codes that we offer does require babel-runtime.
Since the consumers of our package are also supposed to be using React and Babel, it's reasonable to list it as a peer dependency.

HTML output compare

It's 41% less lines compared to previous approach in ic-framework-react:

2017-03-24 5 47 11

The solution is to handle the multi-line parts inside <Text> by converting
it to a vertical Flexbox. That way we don't need to set each row as 'flex-basis: 100%'
thus affect the minimal size for <Text>.
What 'src/Text.js' exports now therefore become 'withStatus(Text)',
so we're exporting the pure <Text> for testing.
@zhusee2 zhusee2 added the wip label Mar 23, 2017
@zhusee2 zhusee2 requested review from paizanmay and cjies March 24, 2017 07:38
@zhusee2 zhusee2 removed the wip label Mar 24, 2017
@@ -1,4 +0,0 @@
// 將您的設定放入此檔案中以覆寫預設值和使用者設定。
Copy link
Contributor

Choose a reason for hiding this comment

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

Add .vscode in gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I just removed it from codebase lol
I think it should be added to the repo if it's used; however it's not used anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

.vscode/ will contain personal workspace settings, I think it should not included in git.

Take a look at the top of Babel-transformed StatusIcon.js:

```js
import _Object$values from 'babel-runtime/core-js/object/values';
import _Object$getPrototypeOf from 'babel-runtime/core-js/object/get-prototype-of';
import _classCallCheck from 'babel-runtime/helpers/classCallCheck';
import _createClass from 'babel-runtime/helpers/createClass';
import _possibleConstructorReturn from 'babel-runtime/helpers/possibleConstructorReturn';
import _inherits from 'babel-runtime/helpers/inherits';
import React, { PureComponent, PropTypes } from 'react';
import icBEM from './utils/icBEM';
```

It's obvious that the ES2015 modules we offer does require both 'babel-runtime' and 'react'.
But usually the consumers of our package uses React and Babel as well.
Therefore it should be fine to list them as peer dependencies

it('renders without crashing', () => {
const div = document.createElement('div');
const element = <TextLabel basic="Basic text" />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should provide more test case to describe how this component works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not write more tests, because I believed that its behaviors have been covered by tests for rowComp() and <Text>.

Since <TextLabel>, for now, is simply a state-less component which renders anything provided by rowComp(), I think tests for it will bring in code duplication.

But I'll add a few more examples and comments to explain how it works.

Copy link
Contributor

@cjies cjies Mar 27, 2017

Choose a reason for hiding this comment

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

Ya, but only renders without crashing is not enough, if we change the internal structure or we remove the rowComp() mixin, this case will pass also.

Maybe we can have some simple test to make sure the internal rowComp() works probably.

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.

LGTM. Thanks 🙌

@zhusee2
Copy link
Contributor Author

zhusee2 commented Mar 27, 2017

🙌🏻

@zhusee2 zhusee2 merged commit 1d3c71b into develop Mar 27, 2017
@zhusee2 zhusee2 deleted the feature/zhusee_text_label branch March 27, 2017 03:54
@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.

2 participants