Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

feat(datepicker): year view, open on focus, bug fixes #8472

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented May 16, 2016

  • Adds the ability for users to switch years quicker by clicking on the month header. Cheers to @mhchen for helping me get started.
  • Adds the ability to open a datepicker by focusing the input.
  • Fixes alignment issues, if the user has set the box-sizing: border-box on everything.
  • Fixes cases where the calendar's width could be set to 0.

Fixes #4251, #4650, #8547, #8030, #8557.

@crisbeto crisbeto added the needs: review This PR is waiting on review from the team label May 16, 2016
@crisbeto
Copy link
Member Author

@ThomasBurleson how should we proceed with reviewing this one? I had to shuffle around a lot of code in order to avoid duplication.

@crisbeto crisbeto force-pushed the datepicker-month branch from a0117df to e5f7b03 Compare May 16, 2016 14:17
if (!self.selectedDate) self.selectedDate = value;

// Also set up the displayDate.
if (!self.displayDate) self.displayDate = self.selectedDate || self.today;
Copy link
Member

Choose a reason for hiding this comment

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

Add braces

@crisbeto crisbeto added needs: work and removed needs: review This PR is waiting on review from the team labels May 18, 2016
@jelbourn
Copy link
Member

High level LGTM from me, thanks!

@crisbeto crisbeto force-pushed the datepicker-month branch from e5f7b03 to d84017a Compare May 21, 2016 09:02
@crisbeto crisbeto added needs: review This PR is waiting on review from the team and removed needs: work labels May 21, 2016
@crisbeto
Copy link
Member Author

crisbeto commented May 21, 2016

@ThomasBurleson @topherfangio @EladBezalel @devversion I've worked through the comments from Jeremy. Can you give me some feedback on this?

var value = this.$viewValue;

// Notify the child scopes of any changes.
self.$scope.$broadcast('md-calendar-parent-changed', value);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should avoid broadcasting, but if it's inevitable I think it should be md-calendar:parent-changed

You can avoid broadcasting by providing a registerChanges that you provide on the controller that gets a callback and saves it in a callback array and when the change happened go through the callback array and call it

Copy link
Member Author

Choose a reason for hiding this comment

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

Doing a registerChanges method is basically the same as broadcasting it on the scope, except that we'd need to handle keeping track of the callbacks and cleaning up afterwards ourselves.

@ThomasBurleson ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels May 25, 2016
@crisbeto crisbeto added needs: work and removed pr: merge ready This PR is ready for a caretaker to review labels May 26, 2016
@crisbeto crisbeto force-pushed the datepicker-month branch from d84017a to 2cbb826 Compare May 26, 2016 19:28
@crisbeto crisbeto force-pushed the datepicker-month branch 2 times, most recently from d7a0236 to 440587c Compare May 27, 2016 18:33
@crisbeto crisbeto self-assigned this May 27, 2016
@crisbeto crisbeto force-pushed the datepicker-month branch 2 times, most recently from 751d6da to 043a818 Compare May 28, 2016 10:01
@crisbeto crisbeto force-pushed the datepicker-month branch 3 times, most recently from 8df4295 to 8226297 Compare May 29, 2016 12:14
@crisbeto crisbeto added needs: review This PR is waiting on review from the team and removed needs: work labels May 29, 2016
@crisbeto
Copy link
Member Author

It should be good to go now @ThomasBurleson.

@topherfangio
Copy link
Contributor

Overall, I think this is really great and helpful!

A couple of thoughts from a UX perspective:

  1. I think the year should have some additional feedback that it is clickable. Not sure the best way to do this, but maybe just changing the background on hover is enough (like we do with the days).
  2. Without an animation between the two, it feels very jumpy and "un-material" IMO. Would it be fairly easy to add a slide or simple fade transition between the two?
  3. Would it be possible to re-use the "day of the week" header area to provide a back button/icon? Right now, because the header disappears, it changes the size of the picker which feels odd, and we also don't give users an easy way to get back to the standard view if they decide they don't want to change anything after all.

Just some of my own personal thoughts 😄

@crisbeto
Copy link
Member Author

@topherfangio

  1. Definitely, but I didn't find a nice way of doing it. I've been trying stuff like this, but I wouldn't say that it looks good. I was leaning more towards something like this.
  2. That sounds interesting! I couldn't get it to work nicely on the first try (for some reason the one with the datepicker didn't transition at all and the one outside a datepicker wasn't rendering the month content immediately), but I might be able to get it working.
  3. I'm not sure about this one, that's a would be a pretty big back button.

@topherfangio
Copy link
Contributor

  1. I think if the button was smaller (like didn't touch the edges) it would work pretty well. I'm not sure the right chevron icon is enough to tell people (unless perhaps it appears only when they hover; I actually like that idea).

  2. I was just thinking a back icon on the left side; the button wouldn't take up the whole width; just a white background with the left chevron/button would be enough 😄

@ThomasBurleson
Copy link
Contributor

@crisbeto - Should i merge this now ?

@crisbeto crisbeto force-pushed the datepicker-month branch from 8226297 to 0bbe9d9 Compare May 31, 2016 05:20
@crisbeto
Copy link
Member Author

I'd say so @ThomasBurleson. Once we have this set up, making improvements should be a bit easier.

* Adds the ability for users to switch years quicker by clicking on the month header. Cheers to @mhchen for helping me get started.
* Adds the ability to open a datepicker by focusing the input.
* Fixes alignment issues, if the user has set the `box-sizing: border-box` on everything.
* Fixes cases where the calendar's width could be set to 0.

Fixes angular#4251, angular#4650, angular#8547, angular#8030, angular#8557.
@crisbeto crisbeto force-pushed the datepicker-month branch from 0bbe9d9 to aaf4686 Compare May 31, 2016 17:01
@crisbeto crisbeto changed the title feat(datepicker): add a year view feat(datepicker): year view, open on focus, bug fixes May 31, 2016
@Splaktar Splaktar added this to the 1.1.0 milestone Mar 28, 2019
@Splaktar Splaktar added type: bug P1: urgent Urgent issues that should be addressed in the next minor or patch release. and removed needs: review This PR is waiting on review from the team labels Mar 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P1: urgent Urgent issues that should be addressed in the next minor or patch release. type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants