Calendar component - Issue #36 (WIP) #45

mAiNiNfEcTiOn · 2017-03-17T12:05:49Z

What does this PR do:

Adds a new helper function that normalizes a Date object's hours, minutes, seconds and milliseconds.
Adds the Calendar component to the kit.
Changes the _default.yaml to contain a basic default style of the calendar.
Changes the builder to ouput errors via util.inspect() which colorizes the output as well
Configures the rule jsx-indent on the .eslintrc to enable the indentLogicalExpressions, overriding its default behavior to the one of v6.10.0: v6.10.1 breaks v6.10.0 builds (related to jsx-indent) jsx-eslint/eslint-plugin-react#1117 (comment)
Locks the snapshots and tests to run on UTC (using the TZ env variable) so that it runs in the same timezone as Travis CI.

Documentation also is a WIP... Won't merge without having it properly set.

Where should the reviewer start:

components/calendar.js

Unit and/or functional tests:

Being added (WIP). But you can start the code review without them :)

coveralls · 2017-03-17T12:08:00Z

Coverage decreased (-11.2%) to 88.797% when pulling 4945877 on add-calendar-component into 0070bb3 on master.

asci · 2017-03-17T12:28:13Z

components/calendar/calendar.js

@@ -0,0 +1,268 @@
+import React from 'react';


why not
import React, { PropTypes, Component } from 'react';?

Indeed... would make it more readable... will do 👍

asci · 2017-03-17T12:31:33Z

components/calendar/calendar.js

+    const { initialDates, maxDate, minDate } = props;
+    const maxLimit = maxDate ? new Date(maxDate) : null;
+    const renderDate = (initialDates && initialDates.length) ? new Date(initialDates[0]) : new Date();
+    const selectedDates = initialDates ? initialDates.map(item => (item ? new Date(item) : null)) : [null, null];


two inlined ternary operators with arrow function - hard to read\understand, please consider simplifying and\or extract

Sure, no problem. 👍

asci · 2017-03-17T12:38:01Z

components/calendar/calendar.js

+  constructor(props) {
+    super();
+
+    const { initialDates, maxDate, minDate } = props;


is it possible by any chance that props will be changed during the lifetime of the component? if so, should you handle it somehow by componentWillReceiveProps?

@asci will add a test for that 👍

asci · 2017-03-17T12:54:20Z

components/calendar/calendar.js

+
+Calendar.defaultProps = {
+  dataAttrs: {},
+  mods: [],


potential reference issue

@asci how come? AFAIK, React's props are immutable since 0.14, are they not?

unless you're not modifying it in the render function it's ok. but somebody else could not notice it and add some
mods.push('some-mod') and it will change mods across all instances of this component. It is not a bug in your code, but potential weak point and I consider as a "better practice" not to put objects\arrays into default props object

Hmmm I see... good point. Will prevent that then :) 👍 thanks.

asci · 2017-03-17T12:55:52Z

components/calendar/panels/days.js

+import { getClassNamesWithMods } from '../../_helpers';
+import DaysView from '../views/days';
+
+export default class Days extends React.Component {


could be replaced by stateless component

Yes indeed... Will take care of that.

asci · 2017-03-17T12:58:37Z

components/calendar/views/days.js

+    const limits = selectedDates.map(item => (item ? item.getTime() : null));
+    const selectedDatesAreEqual = (limits.length === 2) && (limits[0] === limits[1]);
+
+    while (counter < 42) {


Answer to the Ultimate Question of Life, the Universe, and Everything?:D

Well I can add a description but 7 days * 6 rows = 42 options/buttons.

asci · 2017-03-17T13:28:06Z

components/calendar/views/days.js

+    const selectedDatesAreEqual = (limits.length === 2) && (limits[0] === limits[1]);
+
+    while (counter < 42) {
+      const extraClasses = [];


maybe mods?

Yeah I just didn't want it to be confused with the render's mods :)

asci · 2017-03-17T13:28:47Z

components/calendar/views/days.js

+          disabled={isPreviousMonthDisabled}
+          onClick={onNavPreviousMonth}
+          type="button"
+        >&lt;</button>


here should be an icon, right?

Yes :) That's my next challenge 😄 ... Probably will expose via props what you can pass as nav buttons (next and previous).

asci · 2017-03-17T13:36:54Z

themes/_default.yaml

@@ -23,8 +23,11 @@ generic:
    accent-darker: &accent-darker   '#0B4848'
    accent-dark: &accent-dark       '#177D7D'
    accent: &accent                 '#2A9595'
+    accent-border: &accent-border   '#E6BB00'


this is yellow, not Lochinvar color, wich we use for an accent color in the default theme

Not what? :| the accent border was not there I added it lol.

Didn't even knew that we had any specific system to choose the colors 😮 .

Then you'll have to tell me how to select them ;-) (And I have to admit that the calendar colors look a little bit bad now 😄 )

http://paletton.com/palette.php?uid=73i0A0kmYv4gpRFk9G7qbq2qUf4 this was initial color system

@asci are those colors better?

coveralls · 2017-03-17T18:29:52Z

Coverage remained the same at 100.0% when pulling 18eae2f on add-calendar-component into 895767e on master.

coveralls · 2017-03-17T19:08:59Z

Coverage remained the same at 100.0% when pulling 8306d42 on add-calendar-component into 895767e on master.

coveralls · 2017-03-18T08:39:22Z

Coverage remained the same at 100.0% when pulling 3ea23a5 on add-calendar-component into 895767e on master.

mAiNiNfEcTiOn · 2017-03-18T08:56:28Z

Now, let's go onto the icons and accessibility!

coveralls · 2017-03-18T22:03:01Z

Coverage remained the same at 100.0% when pulling 982df55 on add-calendar-component into 895767e on master.

coveralls · 2017-03-18T22:20:51Z

Coverage remained the same at 100.0% when pulling a447aef on add-calendar-component into 895767e on master.

coveralls · 2017-03-19T09:43:01Z

Coverage remained the same at 100.0% when pulling 6754fab on add-calendar-component into 895767e on master.

coveralls · 2017-03-19T09:46:17Z

Coverage remained the same at 100.0% when pulling 2c60581 on add-calendar-component into 895767e on master.

coveralls · 2017-03-19T09:50:57Z

Coverage remained the same at 100.0% when pulling 87342a9 on add-calendar-component into 895767e on master.

coveralls · 2017-03-19T10:12:48Z

Coverage remained the same at 100.0% when pulling 9ce15f9 on add-calendar-component into 895767e on master.

mAiNiNfEcTiOn · 2017-03-19T17:55:51Z

Ok, now tackling the componentWillReceiveProps ... 😄

coveralls · 2017-03-20T15:14:09Z

Coverage remained the same at 100.0% when pulling 786e2e2 on add-calendar-component into 895767e on master.

mAiNiNfEcTiOn · 2017-03-20T15:16:38Z

Ok... now, support for setting the content of the next and previous buttons! 😄

Also, I'll be adding the possibility to set the 'aria-label' sentence.

Adds the Calendar component Updates the example to show the dates (not final) Changes the DaysPanel to export directly the class Makes the code more explicit on the calendar.js Makes the styles customizable Fixes the _default.yaml to be able to build Fixes SCSS to build :D Changes the builder to actually output (and colorize) when an error happens Adds classNames to elements to make them more specific Fixes bug where an error would be triggered when the initialDates attribute was not set. Also resets the minLimit to the passed one when provided via attributes or to null when none is available Hides the nav button when not enabled Changes the _default.yaml to have decent colors for the rendering :P Adds the function leftPad() to the helpers to do left padding of the months and days Improves the code based on the reviews' feedback. Also makes it more testable and fixes some bugs detected when developing the unit tests Converts the DaysPanel to a stateless component with no mods (for now) Improves the code based on reviews' feedback. Also makes it more testable and introduces a new method to merge the locale definitions Removes the enzyme-to-json package and brings jest-serializdr-enzyme which is then configured in the jest config Reverts the removal to give time to migrate the other unit tests. Adds the serializer configuration to the jest.config.json Adds the unit tests and its snapshots Updates the tests for UTC Removes the mods and the dataAttrs from the 'defaultProps'. Updates the snapshots Attempt to set some colors matching the defined color pattern Updates the snapshots Adds some aria-labels to increase accessibility Adds updated snapshots Fixes a lint error :P Adds a function to normalize the dates on the time Changes the code to use the newly added function Updates the tests to use the normalizeDate() and updates the snapshots Moves the 'bind' to its own declaration, to prevent warnings from linting Changed the 'extraClasses' variable to be named 'mods' Changes the 'npm test' to set the testing to be always UTC (to solve CI issues) Also added the TZ=utc to the update-snapshots script Adds a function processProps() to be used by both the constructor and the componentWillReceiveProps() from the Calendar component Adds a wrapper component to make the test of the props being changed. Also adds a test to verify the componentWillReceiveProps. Updates the snapshots Improves coverage of the componentWillReceiveProps Downgraded eslint-plugin-react due to a bug on the jsx-indent rule: jsx-eslint/eslint-plugin-react#1117 Fixes the issue with the eslint-plugin-react's version, enabling the indentLogicalExpressions Adds the possibility to override the display text in the navigation buttons, as well as the aria-label attribute

coveralls · 2017-03-20T20:14:17Z

Coverage remained the same at 100.0% when pulling 71b681b on add-calendar-component into 895767e on master.

coveralls · 2017-03-20T20:15:58Z

Coverage remained the same at 100.0% when pulling 71b681b on add-calendar-component into 895767e on master.

mAiNiNfEcTiOn · 2017-03-20T20:23:28Z

Now, only missing the documentation (finally).

fahad19 · 2017-03-20T20:30:39Z

that's a LOT of work! 👏

…ge calendar with all props set

…so needed to be readjusted

…r math :D

…anges

mAiNiNfEcTiOn · 2017-03-21T10:54:58Z

@asci , @reaktivo , @fahad19, @AlexDudar, @EduardTrutsyk and @iwwwi , can you please review it now?

asci · 2017-03-21T11:18:09Z

Are we going to reuse this calendar on our platfrom apart from home page? So far we have only one place where use calendar.

mAiNiNfEcTiOn · 2017-03-21T11:19:43Z

@asci The CarTrawler widget will have a Search box which will use the calendar as well...

AlexDudar

well done Ricardo!

fahad19 · 2017-03-21T12:10:02Z

components/calendar/calendar.js

+function processProps(props) {
+  const { initialDates, maxDate, minDate, selectionType } = props;
+  const maxLimit = maxDate ? normalizeDate(new Date(maxDate), 23, 59, 59, 999) : null;
+  const renderDate = normalizeDate(((initialDates && initialDates.length && initialDates[0])


does it have any side-effect?

what happens to the original Date object, that is passed to this Calendar component from parent component?

@fahad19 it is mutated. But that's what happens when you call .setHours() or any of it's sister functions :)

But that's why it's called normalizeDate and not returnNewNormalizedDate or something 😄

However, side effects imply that the function is changing things that are not passed to it - implying knowledge of variables outside it's scope, etc - which is not the case.

Regarding your last question, if you check this exact code, it's doing a new Date() of the string coming from the initialDates[0] (when applicable)...

I will improve a little bit the readability.

is there any specific reason for choosing the approach of normalizeDate() over returnNewNormalizedDate()?

@fahad19 check if it's better :)

I agree with you about what you mentioned about side-effects. The only problem we have here is that it is still referencing the parent Component's props.

I would always avoid props mutation of any kind in React. If I really need to transform the value in some way, and expect the Component to re-render on further changes, I would use .setState(), and scope it under the current Component only. Without affecting parent(s).

Similar issue was found in another PR few weeks ago. I think @asci wanted to created a Story out of it so all affected components are updated to avoid this issue.

coveralls · 2017-03-21T12:26:27Z

Coverage remained the same at 100.0% when pulling 0d85c3d on add-calendar-component into 895767e on master.

…enderDate

mAiNiNfEcTiOn · 2017-03-21T12:39:34Z

@AlexDudar thanks ;)

coveralls · 2017-03-21T12:42:31Z

Coverage remained the same at 100.0% when pulling c929a9a on add-calendar-component into 895767e on master.

mAiNiNfEcTiOn added feature request patch labels Mar 17, 2017

mAiNiNfEcTiOn self-assigned this Mar 17, 2017

mAiNiNfEcTiOn requested review from reaktivo, fahad19, asci, AlexDudar, EduardTrutsyk and iwwwi March 17, 2017 12:05

asci reviewed Mar 17, 2017

View reviewed changes

mAiNiNfEcTiOn force-pushed the add-calendar-component branch from a6ca8d3 to 3512d2e Compare March 17, 2017 18:06

mAiNiNfEcTiOn force-pushed the add-calendar-component branch from ca19875 to 71b681b Compare March 20, 2017 20:14

Ricardo Machado added 4 commits March 21, 2017 11:51

Adds the examples with a normal calendar with all props set and a ran…

cb93c82

…ge calendar with all props set

Fixes issue where renderDate was not between the min and max limits, …

2a0146f

…so needed to be readjusted

Fixes issue where in the edge-months (0 and 11) we were doing imprope…

01bc264

…r math :D

Updates the snapshots and the tests to cover properly with the new ch…

201061b

…anges

AlexDudar approved these changes Mar 21, 2017

View reviewed changes

fahad19 reviewed Mar 21, 2017

View reviewed changes

fahad19 approved these changes Mar 21, 2017

View reviewed changes

Fixes poor configuration on the .eslintrc

0d85c3d

asci approved these changes Mar 21, 2017

View reviewed changes

Improved readability on the ternary operator of the handling of the r…

c929a9a

…enderDate

mAiNiNfEcTiOn merged commit c83a57f into master Mar 21, 2017

mAiNiNfEcTiOn deleted the add-calendar-component branch March 21, 2017 12:50

asci mentioned this pull request Apr 3, 2017

Dropdown and Checkbox component #49

Merged

asci mentioned this pull request May 5, 2017

Improvements for CollapseItem component #76

Merged

mAiNiNfEcTiOn added this to Done in Components Sep 7, 2017

Calendar component - Issue #36 (WIP) #45

Calendar component - Issue #36 (WIP) #45

Conversation

mAiNiNfEcTiOn commented Mar 17, 2017 • edited

What does this PR do:

Where should the reviewer start:

Unit and/or functional tests:

coveralls commented Mar 17, 2017

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

mAiNiNfEcTiOn Mar 17, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 17, 2017

coveralls commented Mar 17, 2017

coveralls commented Mar 18, 2017

mAiNiNfEcTiOn commented Mar 18, 2017

coveralls commented Mar 18, 2017

coveralls commented Mar 18, 2017

coveralls commented Mar 19, 2017

coveralls commented Mar 19, 2017

coveralls commented Mar 19, 2017

coveralls commented Mar 19, 2017

mAiNiNfEcTiOn commented Mar 19, 2017

coveralls commented Mar 20, 2017

mAiNiNfEcTiOn commented Mar 20, 2017

coveralls commented Mar 20, 2017

coveralls commented Mar 20, 2017

mAiNiNfEcTiOn commented Mar 20, 2017

fahad19 commented Mar 20, 2017

mAiNiNfEcTiOn commented Mar 21, 2017

asci commented Mar 21, 2017 • edited

mAiNiNfEcTiOn commented Mar 21, 2017

AlexDudar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mAiNiNfEcTiOn Mar 21, 2017 • edited

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

coveralls commented Mar 21, 2017

mAiNiNfEcTiOn commented Mar 21, 2017

coveralls commented Mar 21, 2017

mAiNiNfEcTiOn commented Mar 17, 2017 •

edited

mAiNiNfEcTiOn Mar 17, 2017 •

edited

asci commented Mar 21, 2017 •

edited

mAiNiNfEcTiOn Mar 21, 2017 •

edited