Skip to content

Conversation

@nicolethoen
Copy link
Contributor

@nicolethoen nicolethoen commented Feb 21, 2018

Description

fixes #711
Styled the Angular UI datepicker so that it looks more like the patternfly datepicker.
Mostly removing the week numbers from the beginning of each week and styling the various buttons.
The user can also still pass dateOptions to the Angular UI datepicker to customize its behavior.

screen shot 2018-02-21 at 11 47 25 am 2

screen shot 2018-02-22 at 2 24 24 pm 2

screen shot 2018-02-21 at 11 48 10 am 2

PR Checklist

  • Unit tests are included
  • Screenshots are attached (if there are visual changes in the UI)
  • A Designer (@beanh66) is assigned as a reviewer (if there are visual changes in the UI)
  • A CSS rep (@cshinn) is assigned as a reviewer (if there are visual changes in the UI)

@beanh66
Copy link
Member

beanh66 commented Feb 21, 2018

@nicolethoen A couple of minor things:

  • Seems like we need more spacing around the month name
  • The fa-angle-double-left and right icons should be used for advancing through months
  • The selected date should be shown in this format: 02/21/2018
  • Today should not be bold and should match the font size of "February"

I can't tell by your images if you have hover states on the month selection and the "Today" selection but those should match the core implementation in PatternFly if possible.

screen shot 2018-02-21 at 1 43 43 pm

Thanks!

Copy link
Member

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

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

Requested minor visual changes

* @name patternfly datepicker
*
* @description
* Select module for patternfly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add description apropos for the control, eg.

Datepicker module for patternfly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch. and sublime use of apropos.

*
* @param {date} date must be a Javascript Date - to be displayed in the input. Can be left empty.
* @param {string} format default date format for displayed dates
* @param {object} dateOptions an object to configure the uib-datepicker in one place (optional)
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is an object, some reference material might be helpful. See other object descriptions which describe the format/syntax of the options allowed for date.

* @param {string} format default date format for displayed dates
* @param {object} dateOptions an object to configure the uib-datepicker in one place (optional)
* @param {boolean} isOpen whether or not to show the datepicker (default false) (optional)
* @param {object} popupPlacement Placement of the datepicker popup relative to the input element. Passing in 'auto' separated by a space bfore the placement will enable auto positioning. (optional)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above vis a vis the object description, but additionally, with this option it might be nice to see the allowance for different placements in the demo area below. ie a radio button for say above/below.

jcaianirh
jcaianirh previously approved these changes Feb 21, 2018
Copy link
Contributor

@jcaianirh jcaianirh left a comment

Choose a reason for hiding this comment

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

Minor comments. Looks awesome Nicole!!

* @param {object} popupPlacement Placement of the datepicker popup relative to the input element. Passing in 'auto' separated by a space bfore the placement will enable auto positioning. (optional)
*
* @description
* A wrapper for the Angular UI {@link http://angular-ui.github.io/bootstrap/#!#datepickerPopup datepicker}.
Copy link
Member

Choose a reason for hiding this comment

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

That link isn't working for me, it goes to the top of Angular UI Bootstrap page. I think http://angular-ui.github.io/bootstrap/#datepickerPopup will be enough to link-n-launch to the correct part of the page.
Hmm..still not working in some cases. Funny, even their GitHub page's 'here' and 'inline datepicker' links do not work correctly for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah - i've noticed the same problem as I've tried linking to the param documentation in response to Joe's comments... Not sure what to do about that.


&.active {
background: @color-pf-blue-400;
border-color: #003d5f;
Copy link
Member

Choose a reason for hiding this comment

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

This should be a color from the patternfly color palette.

bindings: {
format: '@',
date: '<',
dateOptions: '@',
Copy link
Member

Choose a reason for hiding this comment

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

dateOptions is an object, not a string, so it needs to be defined as dateOptions: '<'. You'll notice when you make this change, your overrides in your example no longer work Ex:

$scope.dateOptions = {
        showWeeks : true
      };

:-(

This is because the $onInit() is actually called after the objects from the example are bound. Try adding this to your component:

    ctrl.$onChanges = function (changesObj) {
      console.log("on changes");
    };

Then in the browser, source tab, open up angular-patternfly.js, find the component, set breakpoints in $onInit() and $onChanges(). You may want to remove the first datepicker example, just to see how the lifecycle of one example works. You'll see that $onChanges() is called first with the objects from the example, then $onInit() is called afterwards -in this case overriding the $scope.dateOptions set by the user/example.

We typically have some sort of if (!angular.isDefined(...).. or 'defaulting' operation in the $onInit() to prevent overriding.

Here's some code which has some examples of $onInit(), $onChanges, and even $doCheck for deep comparisions.

Here is another code example where we use lodash ._defaults(...) to handle default props and user defined props.

Copy link
Member

Choose a reason for hiding this comment

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

^^ I'd probably stick with the last example and lodash ._defaults(...) it's a nice solution.

Copy link
Member

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

Pls see previous comment about bindings

@dtaylor113
Copy link
Member

You also have to change your commit msg to be something like this. I think feat(pfBootstrapDatePicker): Initial Implementation

beanh66
beanh66 previously approved these changes Feb 22, 2018
Copy link
Member

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

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

@nicolethoen The visual changes look good based on your updated screenshot. Thanks!

@dtaylor113
Copy link
Member

Hi @nicolethoen, latest code looks good! Here are my findings on updating attributes:

$timeout(function () {
  $scope.dateOptions.showWeeks = false;  // doesn't take effect on open popup, but does if re-opened
  $scope.popupPlacement = "upper-right"; // doesn't take effect on open popup, but does if re-opened
  // These do take effect immediately in open popup. 
  $scope.isOpen = "";
  $scope.date = new Date("Dec 1, 2000");
  $scope.format1 = "MM-dd-yy";
}, 2000);

So, for this particular component/usecase I think these are acceptable.

@dtaylor113
Copy link
Member

Can you pls look into changing the selected color to match pf design:

PF Design:
image

Current Impl:
image

The pf core example is showing the correct color: http://www.patternfly.org/pattern-library/forms-and-controls/datepicker/#code

@@ -0,0 +1,78 @@
describe('Component: pfBootstrapDatepicker', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Appreciate the unit tests! 👍

@nicolethoen
Copy link
Contributor Author

The pf core example is showing the correct color: http://www.patternfly.org/pattern-library/forms-and-controls/datepicker/#code

so, the yellow color here is actually highlighting today's date - not the selected date.

For some reason, the angular ui component doesn't distinguish between todays date and your selected date. So it will show today's date as if it were selected in blue until you select a date and it will highlight that in blue. I've had a hard time getting the yellow to stick to todays date, but I'll keep trying.

@dtaylor113
Copy link
Member

I've had a hard time getting the yellow to stick to todays date, but I'll keep trying.

Ok, sounds good. Not a show stopper :-)

@dtaylor113
Copy link
Member

@nicolethoen The visual changes look good based on your updated screenshot. Thanks!

Hi @beanh66, could you please approve this PR if satisfied with the visuals. -thanks!

@jeff-phillips-18, pls review again, I think your issue(s) have been addressed -thanks!

dtaylor113
dtaylor113 previously approved these changes Feb 23, 2018
Copy link
Member

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

Assuming commits are squashed and commit msg is in the form of 'feat(...)'. Nice job, thanks!

@beanh66
Copy link
Member

beanh66 commented Feb 23, 2018

@dtaylor113 @nicolethoen Yes this satisfies the visuals from what I can tell. I can't see if there are hover colors on the month and the current date. Are those matching the design? And does the month selection open up to a display of all months? Not sure if there is somewhere I can check out the current implementation. If not, the screenshots work. Thanks!

@nicolethoen
Copy link
Contributor Author

@beanh66 @dtaylor113

Here i've tried capturing the latest changes with hover states.
screen shot 2018-02-23 at 3 02 18 pm
screen shot 2018-02-23 at 3 02 29 pm
screen shot 2018-02-23 at 3 02 41 pm

@nicolethoen nicolethoen force-pushed the pfBootstrapDatepicker branch from 501a515 to 3ad3395 Compare February 23, 2018 20:21
@nicolethoen nicolethoen merged commit 5aac445 into patternfly:master Feb 26, 2018
@nicolethoen nicolethoen deleted the pfBootstrapDatepicker branch February 26, 2018 12:44
@beanh66
Copy link
Member

beanh66 commented Feb 26, 2018

Thanks @nicolethoen! Hover states look great. It looks like the version with years or months should have more space around them and in the PF core implementation the months are abbreviated. Is that possible here? Posted a screenshot below:
screen shot 2018-02-26 at 8 10 23 am

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.

angular-ui-bootstrap Datepicker component does not include PatternFly styling

5 participants