[Badge] Port Badge Component #6043

stunaz · 2017-01-31T04:45:16Z

PR has tests / docs demo, and is linted.
Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

oliviertassinari

That looks pretty cool so far. Thanks for contributing to the next effort!
I couldn't review it in detail. Will do it later.
It would be awesome if you could add a visual regression test.

oliviertassinari · 2017-01-31T17:33:12Z

src/Badge/Badge.spec.js

+
+    assert.strictEqual(wrapper.contains(testChildren), true, 'should contain the children');
+    assert.strictEqual(wrapper.node.props.style.backgroundColor, style.backgroundColor,
+    'should overwrite badge backgroundColor');


I think that our convention is the following one:

assert.strictEqual(wrapper.node.props.style.backgroundColor, style.backgroundColor, 'should overwrite badge backgroundColor');

You mean the spaces? if so I will change that.

Yes, it's about the spacing.

oliviertassinari · 2017-01-31T17:33:49Z

src/Badge/Badge.js

+    badgeContent,
+    className: classNameProp,
+    children,
+    primary, // eslint-disable-line no-unused-vars


no need for // eslint-disable-line no-unused-vars

oliviertassinari · 2017-01-31T17:33:59Z

src/Badge/Badge.js

+    className: classNameProp,
+    children,
+    primary, // eslint-disable-line no-unused-vars
+    accent, // eslint-disable-line no-unused-vars


no need for // eslint-disable-line no-unused-vars

oliviertassinari · 2017-01-31T17:34:16Z

src/Badge/Badge.js

+import customPropTypes from '../utils/customPropTypes';
+
+const radius = 12;
+const radius2x = Math.floor(2 * radius);


Math.floor(?

Don't really know, took it from the master branch

@stunaz It rounds down to the nearest integer, which doesn't make much sense for integer multiplication. 😄

I get that, so I just write const radius2x = 2 * radius;, will that be ok?

@stunaz Sounds good.

mbrookes · 2017-01-31T20:48:19Z

src/Badge/Badge.js

+  /**
+   * The badge will be added relativelty to this node.
+   */
+  children: PropTypes.node,


Should children be required? Not sure a badge makes much sense without...

indeed, makes sense. I will mark it as required.

mbrookes · 2017-02-01T01:17:37Z

@stunaz thanks for picking up the migration of this component - Tooltip has your name all over it for the next one! 😆

While we have the opportunity though, I think it would be worth discussing what a badge should do and look like. I've never liked the large separation between the badge and its child, so I went back to the source (Material Design spec), and discovered that, at least as far as is searchable, there is none. The only reference to badge is in FAB, saying not to add one. (If there are examples in the spec, so much the better - I didn't browse every page looking for one).

As a next-best-thing, here's what MDL have done:

That overlap with the child component seems quintessential for a badge.

Secondly, I'd question the validity of some of our current examples:

Should a badge be applied to an icon that is smaller than the badge itself?
Should a badge be an IconButton?
Should a badge be used as a substitute for superscript? ( (c) symbol example. )

Lets see if we can improve this as part of the migration, otherwise it's likely to be left for the forseeable.

- set children as required prop - remove useless lint comment

mbrookes · 2017-02-01T01:29:09Z

docs/api/Badge/Badge.md

@@ -11,7 +11,7 @@ Props
 | accent | bool | false | If true, the badge will use the accent badge colors. |
 | badgeClassName | string |  | The CSS class name of the badge element. |
 | <span style="color: #31a148">badgeContent *</span> | node |  | This is the content rendered within the badge. |
-| children | node |  | The badge will be added relativelty to this node. |
+| <span style="color: #31a148">children *</span> | node |  | The badge will be added relativelty to this node. |


Typo in relatively, but it should probably be relative, and even then the sentence is awkward.

How about simply "The node that the badge will be applied to."

stunaz · 2017-02-01T02:00:45Z

@mbrookes Tooltip 😃 , I can certainly try, but I think it is a big one as it involves a lot of things like performance for starter.

I surely can work on the applied styles and improve the examples.

Should a badge be applied to an icon that is smaller than the badge itself?

Maybe this dicision should be on the userland ?

Should a badge be an IconButton?

Interesting ... I tried, it works, but it can look ugly if primary or accent are applied to the badge

Should a badge be used as a substitute for superscript? ( (c) symbol example. )

I don't see why not

mbrookes · 2017-02-01T02:26:32Z

it involves a lot of things like performance

Mm, right I'd forgotten about that concern.

Maybe this dicision should be on the userland ?

Yes, but we shouldn't really be demonstrating an example of poor usage.

it works

Yes, but again I don't think it represents good practice, so shouldn't be a docs example.

I don't see why not

Because there is already a semantically valid way of achieving that? It's another questionable example IMHO.

Appreciate you inherited these issues from master!

stunaz · 2017-02-01T02:47:23Z

Maybe we drop the further example, and show only the simple example ?
What about something like this:

stunaz · 2017-02-01T02:50:31Z

@oliviertassinari @mbrookes Can you please point me how to make visual regression test ?

mbrookes · 2017-02-01T20:34:50Z

What about something like this

Much better! Perhaps an appropriate color / shade of grey for the main icons rather than black?

oliviertassinari · 2017-02-02T20:08:25Z

@stunaz Do you need any help? I'm not sure what's preventing us from moving forward.

stunaz · 2017-02-02T20:20:41Z

@oliviertassinari I will try and add a visual regression test tonight. @mbrookes had concerrn about what the examples that should be shown. What do you want me to do about that?

stunaz · 2017-02-03T13:07:19Z

I couldnt make it work, looks like I have an env issue :
Running the docker, I kind of have an error at the end:

Running the test regressions, I got:

apart from the visual regression test, is there something else that I can do?

oliviertassinari

I agree with @mbrookes, the examples of material-design-lite are just great. I think that we can keep a single Simple examples demo following mdl.
Regarding the visual regressions, do you have any error message?

oliviertassinari · 2017-02-04T00:13:39Z

docs/site/src/demos/badges/SimpleBadge.js

+import NotificationIcon from 'material-ui/svg-icons/notification';
+
+const styleSheet = createStyleSheet('SimpleBadge', () => ({
+  badge: {


Two points here:

That style doesn't have enough specificity to be applied. It's a dead style
You need to add the badge component into the MUI_SHEET_ORDER.

Users shouldn't have to add that style. It should be handled by the badge.

hmmm @oliviertassinari , I didn't get that.

I am not sure what MUI_SHEET_ORDER is for? You mean adding it in docs/site/src/components/App?

That style is not needed, it is just for the demo. I have updated the example code, have a look

oliviertassinari · 2017-02-04T00:16:03Z

src/Badge/Badge.js

+    root: {
+      position: 'relative',
+      display: 'inline-block',
+      padding: `${radius2x}px ${radius2x}px ${radius}px ${radius}px`,


I agree with @mbrookes. I don't think that the badge should impact the layout of the element.
What about removing that padding property?

agree too. done

oliviertassinari · 2017-02-04T00:17:16Z

src/Badge/Badge.js

+      alignContent: 'center',
+      alignItems: 'center',
+      position: 'absolute',
+      top: 0,


What about?

top: -radius,

oliviertassinari · 2017-02-04T00:17:24Z

src/Badge/Badge.js

+      alignItems: 'center',
+      position: 'absolute',
+      top: 0,
+      right: 0,


What about?

right: -radius,

stunaz · 2017-02-06T01:38:58Z

@oliviertassinari I have done some modification, here is the new look of the page:

Regarding the visual regression test it is not working, here is the logs that I think explain the issue:
Are you guys working on windows ? ( I am on mac)

Looks good :)

oliviertassinari · 2017-02-06T23:08:27Z

src/Badge/Badge.spec.js

+    );
+
+    assert.strictEqual(wrapper.contains(testChildren), true, 'should contain the children');
+    assert.strictEqual(wrapper.node.props.style.backgroundColor, style.backgroundColor,


assert.strictEqual(wrapper.props().style.backgroundColor, style.backgroundColor,

oliviertassinari · 2017-02-06T23:12:17Z

Regarding the visual regression test it is not working, here is the logs that I think explain the issue:
Are you guys working on windows ? ( I am on mac)

We are on a Mac too. I have the same issues on my docker logs. I don't think that the issue is on that end. I can take care of generating the visual diff this time :). Could you just fix the linting issue?

stunaz · 2017-02-07T00:22:26Z

there are no more linting issue, the ci because fails because of the image missing, I can remove the regression test

oliviertassinari

@stunaz I'm gonna do a second iteration on this PR to fix the build. Thanks a lot!

[Badge] Port Badge Component

09250d6

muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Jan 31, 2017

[Badge] Port Badge Component

6ab0e9c

muibot added the PR: Needs Review label Jan 31, 2017

[Badge] Port Badge Component

df9bf25

oliviertassinari added component: badge This is the name of the generic UI component, not the React module! next labels Jan 31, 2017

oliviertassinari reviewed Jan 31, 2017

View reviewed changes

oliviertassinari removed the PR: Needs Review label Jan 31, 2017

mbrookes reviewed Jan 31, 2017

View reviewed changes

- lint convention

c4c17ce

- set children as required prop - remove useless lint comment

muibot added the PR: Needs Review label Feb 1, 2017

mbrookes reviewed Feb 1, 2017

View reviewed changes

docs

11c87b6

oliviertassinari mentioned this pull request Feb 1, 2017

[docs] Explain how to use the visual regression tests #6050

Merged

3 tasks

oliviertassinari force-pushed the next branch 6 times, most recently from cd92911 to 2f73c43 Compare February 2, 2017 19:59

oliviertassinari previously requested changes Feb 4, 2017

View reviewed changes

oliviertassinari removed the PR: Needs Review label Feb 4, 2017

adjusted the examples

852cffe

muibot added the PR: Needs Review label Feb 6, 2017

Fix lint issue

8fcaca0

oliviertassinari reviewed Feb 6, 2017

View reviewed changes

oliviertassinari approved these changes Feb 7, 2017

View reviewed changes

oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 7, 2017

oliviertassinari merged commit 9a50136 into mui:next Feb 7, 2017

muibot added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 7, 2017

[Badge] Port Badge Component #6043

[Badge] Port Badge Component #6043

Conversation

stunaz commented Jan 31, 2017 • edited by oliviertassinari Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes commented Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

stunaz commented Feb 1, 2017 • edited Loading

mbrookes commented Feb 1, 2017 • edited Loading

stunaz commented Feb 1, 2017

stunaz commented Feb 1, 2017

mbrookes commented Feb 1, 2017

oliviertassinari commented Feb 2, 2017

stunaz commented Feb 2, 2017

stunaz commented Feb 3, 2017

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stunaz commented Feb 6, 2017

Choose a reason for hiding this comment

oliviertassinari commented Feb 6, 2017 • edited Loading

stunaz commented Feb 7, 2017

oliviertassinari left a comment

Choose a reason for hiding this comment

stunaz commented Jan 31, 2017 •

edited by oliviertassinari

Loading

mbrookes commented Feb 1, 2017 •

edited

Loading

stunaz commented Feb 1, 2017 •

edited

Loading

mbrookes commented Feb 1, 2017 •

edited

Loading

oliviertassinari commented Feb 6, 2017 •

edited

Loading