Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IgxTimePicker refactoring #8947

Merged
merged 53 commits into from
Apr 19, 2021
Merged

Conversation

PlamenaMiteva
Copy link
Contributor

@PlamenaMiteva PlamenaMiteva commented Feb 10, 2021

Closes #6482

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

…iteui-angular into PMiteva/time-picker-refactoring

# Conflicts:
#	projects/igniteui-angular/src/lib/time-picker/time-picker.component.ts
@jackofdiamond5 jackofdiamond5 mentioned this pull request Mar 12, 2021
14 tasks
…iteui-angular into PMiteva/time-picker-refactoring

# Conflicts:
#	projects/igniteui-angular/src/lib/date-common/pickers-base.directive.ts
…iteui-angular into PMiteva/time-picker-refactoring

# Conflicts:
#	projects/igniteui-angular/src/lib/date-common/picker-base.directive.ts
#	projects/igniteui-angular/src/lib/date-picker/date-picker.utils.ts
#	projects/igniteui-angular/src/lib/time-picker/time-picker.common.ts
#	projects/igniteui-angular/src/lib/time-picker/time-picker.component.spec.ts
#	projects/igniteui-angular/src/lib/time-picker/time-picker.component.ts
#	projects/igniteui-angular/src/lib/time-picker/time-picker.directives.ts
#	src/app/time-picker/time-picker.sample.ts
…IgniteUI/igniteui-angular into PMiteva/time-picker-refactoring

# Conflicts:
#	projects/igniteui-angular/src/lib/date-common/picker-base.directive.ts
@@ -359,6 +360,7 @@ export abstract class DateTimeUtil {
const format = timePart.replace(/\d/g, '0');
timePart = new MaskParsingService().replaceInMask(timePart, value,
{ format, promptChar: '' }, 0, value.length).value;
timePart = timePart.substr(0, timePart.length - 1);
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, taking everything except the last char won't work in too many scenarios - time portion has zone different than UTC (+00) or no zone at all in which case you'd be removing from the milliseconds most likely. What's more, I don't see the need to remove parts of the value or go as far as the mask parsing for this - you already have a date-only part, perhaps just combining it with the literal and the time portion will produce a valid date string?

Copy link
Member

Choose a reason for hiding this comment

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

Also, might want to add some invalid date checks to this parse method, unless the consuming locations are supposed to check for that. The return type is described as Date | null but I don't see how the latter is possible in the current code

@Lipata Lipata added the squash-merge Merge PR with "Squash and Merge" option label Apr 15, 2021
[attr.role]="timeItem.isSelectedTime ? 'spinbutton' : null"
[attr.aria-valuenow]="timeItem.isSelectedTime ? timeItem.hourValue : null"
[attr.aria-valuemin]="timeItem.isSelectedTime ? timeItem.minValue : null"
[attr.aria-valuemax]="timeItem.isSelectedTime ? timeItem.maxValue : null"
Copy link
Member

Choose a reason for hiding this comment

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

Since the time item is apparently aware if it's selected, these can all be host bound in the directive instead to reduce the amount of repetition in this template

*/
@Input()
public disabled = false;
public displayFormat: string;
Copy link
Member

Choose a reason for hiding this comment

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

Technically, that's inherited already, though just for the documentation it might be okay to keep. Same with a few others.

@Lipata Lipata merged commit b2b1758 into pickers-refactoring Apr 19, 2021
@Lipata Lipata deleted the PMiteva/time-picker-refactoring branch April 19, 2021 14:08
Lipata pushed a commit that referenced this pull request Apr 21, 2021
…#6483 (#9160)

* feat(*): add PickersBaseDirective

* feat(date-time-editor): handling for wheel events

* feat(types): add new enum HeaderOrientation

* feat(date-time-editor): add preventSpeenOnWheel input

* feat(date-time-editor): can be set to suppress focus

* feat(DatePickerUtils): add method for min/max validation

* refactor(CalendarContainer): move to date-utils

* feat(PickersBaseDirective): implements EditorProvider

* refactor(picker-icons): move picker icon components to date-common

* feat(date-time-editor): add option to set spin delta per part #7169 (#8987)

* refactor(date-range): pickers base, overlay service, template buttons, valueChange

* refactor(date-editor): rename isSpinLoop to spinLoop

* feat(date-time-editor, date-range-picker, date-picker): ISO 8601 support #6994

* feat(date-picker): add readonly prop, close on escape

* refactor(advanced-filtering): update date picker template

* refactor(filtering-row): update date picker template

* refactor(excel-style-date-expr): update date picker template

* refactor(input-directive):  set disabled using hostbinding

* feat(igx-time-picker): refactoring #6482 (#8947)

* feat(date-time-editor, pickers): add migrations, changelog, readme #6482, #6483 (#9319)

Co-authored-by: Boris <jackofdiamond596@gmail.com>
Co-authored-by: plamenamiteva <plamenamiteva@abv.bg>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕐 time-picker refactoring squash-merge Merge PR with "Squash and Merge" option
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants