Default profile colors #27

rgommezz · 2017-07-28T12:31:57Z

After sampling some images, I extrapolated some greyscale colors as defaults for each profile.
See #2

This PR merges the palette instance with the defaults, before the event emitter publish the change to all subscribers.

TODO

Validate this.props.defaults
Fix Flow
Unit test createMaterialPalette
Defaults should be only provided for the types passed
Unit test defaults merging in PaletteProvider

Note: I'll fix flow issues before merging this...couldn't run it again locally

zamotany

Missing PaletteProvider test cases. Minimal CC - 100% 😛

zamotany · 2017-07-30T17:44:25Z

src/PaletteProvider.js

+
+  _validateDefaults() {
+    if (this.props.defaults) {
+      if (typeof this.props.defaults !== 'object') {


Can you flatten the conditionals?

if (this.props.defaults && typeof this.props.defaults !== 'object')

That part is not finished, I am about to complete the defaults validation today

zamotany · 2017-07-30T17:45:03Z

src/PaletteProvider.js

+        const paletteWithDefaults = this._mergeWithDefaults(palette);
+        execIfFunction(this.props.onFinish, paletteWithDefaults);
        if (!this.props.forceRender) {
          this.setState({ palette });


You should pass paletteWithDefaults instead of palette

zamotany · 2017-07-30T17:46:27Z

src/PaletteProvider.js

+      ),
+    };
+    return {
+      ...Object.keys(palette)


You don't need to spread the object inside object -> you're reducing it to object already.

@zamotany I think it's because he is merging it with defaults

a ok, haven't seen that ...defaults

satya164 · 2017-08-01T11:19:44Z

src/constants/defaults.js

+const defaultLightMuted = '#BDBDBD';
+const defaultDarkMuted = '#616161';
+
+export const validColorProfiles = [


You can use an object here and reuse it for the typedef:

const validColorProfiles = { vibrant: true, lightVibrant: true, darkVibrant: true, muted: true, lightMuted: true, darkMuted: true, }; type ColorProfile = $Keys<typeof validColorProfiles>;

coveralls · 2017-08-01T12:51:23Z

Coverage increased (+0.7%) to 92.593% when pulling 6f59268 on default_profile_colors into 8ad6872 on master.

coveralls · 2017-08-01T13:01:31Z

Coverage increased (+0.7%) to 92.531% when pulling 0f1c7c1 on default_profile_colors into 8ad6872 on master.

coveralls · 2017-08-01T13:48:01Z

Coverage increased (+0.7%) to 92.531% when pulling c8de240 on default_profile_colors into 8ad6872 on master.

coveralls · 2017-08-01T13:55:12Z

Coverage increased (+0.7%) to 92.531% when pulling 3b70ee7 on default_profile_colors into 8ad6872 on master.

satya164 · 2017-08-01T14:18:58Z

src/utils/validate.js

    const validProfilesKeys = ['bodyTextColor', 'color', 'titleTextColor'];
    (Object.keys((defaults: any)): ColorProfile[]).forEach(profile => {
-      if (!validColorProfiles.includes(profile)) {
+      if (!Object.keys(validColorProfiles).includes(profile)) {


Or profile in validColorProfiles

coveralls · 2017-08-01T14:44:09Z

Coverage increased (+5.6%) to 97.51% when pulling 41c95bd on default_profile_colors into 8ad6872 on master.

rgommezz · 2017-08-02T09:26:09Z

@satya164 @zamotany you can take another look 🤖

zamotany · 2017-08-02T11:08:15Z

src/__tests__/createMaterialPalette.test.js

+        },
+      }));
+
+    createPalette({


You need to return the Promise from createPalette otherwise the test will assume it's sync and won't wait for it.

coveralls · 2017-08-03T09:28:27Z

Coverage increased (+5.6%) to 97.51% when pulling 95d5ae4 on default_profile_colors into 8ad6872 on master.

coveralls · 2017-08-03T09:31:49Z

Coverage increased (+5.6%) to 97.51% when pulling f564ad1 on default_profile_colors into 8ad6872 on master.

coveralls · 2017-08-03T09:33:06Z

Coverage increased (+5.6%) to 97.51% when pulling 8054f5b on default_profile_colors into 8ad6872 on master.

coveralls · 2017-08-03T12:10:41Z

Coverage increased (+6.9%) to 98.77% when pulling 6ba98c4 on default_profile_colors into 8ad6872 on master.

coveralls · 2017-08-03T12:25:16Z

Coverage increased (+7.3%) to 99.18% when pulling 1578bf5 on default_profile_colors into 8ad6872 on master.

satya164 · 2017-08-03T12:36:34Z

You guys are crazy

coveralls · 2017-08-03T12:46:22Z

Coverage increased (+8.1%) to 100.0% when pulling 57e1576 on default_profile_colors into 8ad6872 on master.

coveralls · 2017-08-03T12:48:51Z

Coverage increased (+8.1%) to 100.0% when pulling 993bfe1 on default_profile_colors into 8ad6872 on master.

zamotany · 2017-08-03T13:01:50Z

src/utils/__tests__/validateCreatePalette.test.js

@@ -0,0 +1,56 @@
+/* eslint-disable import/first */
+jest.mock('../validateCreatePaletteArgs', () => ({


jest.mock('../validateCreatePaletteArgs') is sufficient

zamotany

👍

rgommezz added 2 commits July 28, 2017 12:58

feat(defaultProfiles): Providing default profiles

43c6a4f

feat(defaultProfiles): Merging defaults in PaletteProvider

d81f1a8

rgommezz requested review from satya164 and zamotany July 28, 2017 12:31

Addressing flow issues

0f26995

satya164 approved these changes Jul 30, 2017

View reviewed changes

zamotany suggested changes Jul 30, 2017

View reviewed changes

rgommezz changed the title ~~Default profile colors~~ WIP - Default profile colors Jul 31, 2017

rgommezz and others added 9 commits August 1, 2017 11:20

Passing palette with defaults to connected components and event emitter

0f49067

Attempting to solve flow errors

665bfeb

Small fix

2d564b4

fix flow error

d263d35

fix and remove flow ignore comment

0d2a8d0

remove globalDefaults from withPalette

d971eef

replace default export with named one for create function

bb7cd13

this.props.defaults validator function

b857638

Unit tests for validateDefaults

49f5495

satya164 reviewed Aug 1, 2017

View reviewed changes

rgommezz added 3 commits August 1, 2017 14:26

Fixing unit tests

7a92c47

Disabling require flow parameter type rule

b1434b4

Defaults should only be provied for the types specified by the user

6f59268

Separating createMaterialPalette into its own module

0f1c7c1

Unit test for PaletteProvider merge with defaults

c8de240

Not duplicating logic

3b70ee7

satya164 requested changes Aug 1, 2017

View reviewed changes

Unit test for createMaterialPalette

41c95bd

rgommezz changed the title ~~WIP - Default profile colors~~ Default profile colors Aug 2, 2017

zamotany suggested changes Aug 2, 2017

View reviewed changes

Adding artifacts and fixing async test for palette

95d5ae4

Trying out different path for coverage

f564ad1

Trying out different path for coverage

8054f5b

100% test coverage on utils

6ba98c4

100% test coverage for createMaterialPalette

1578bf5

satya164 approved these changes Aug 3, 2017

View reviewed changes

100% coverage, fucking genious?

57e1576

removing console.log

993bfe1

zamotany suggested changes Aug 3, 2017

View reviewed changes

simplifying mock

fef3d01

zamotany approved these changes Aug 3, 2017

View reviewed changes

rgommezz merged commit ec73f4c into master Aug 3, 2017

rgommezz deleted the default_profile_colors branch August 3, 2017 13:05

		@@ -0,0 +1,56 @@
		/* eslint-disable import/first */
		jest.mock('../validateCreatePaletteArgs', () => ({

Default profile colors #27

Default profile colors #27

Uh oh!

Conversation

rgommezz commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zamotany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 1, 2017

Uh oh!

coveralls commented Aug 1, 2017

Uh oh!

coveralls commented Aug 1, 2017

Uh oh!

coveralls commented Aug 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 1, 2017

Uh oh!

rgommezz commented Aug 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 3, 2017

Uh oh!

coveralls commented Aug 3, 2017

Uh oh!

coveralls commented Aug 3, 2017

Uh oh!

coveralls commented Aug 3, 2017

Uh oh!

coveralls commented Aug 3, 2017

Uh oh!

satya164 commented Aug 3, 2017

Uh oh!

coveralls commented Aug 3, 2017

Uh oh!

coveralls commented Aug 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zamotany left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rgommezz commented Jul 28, 2017 •

edited

Loading