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

feat(dateRangePicker): Adding DateRangePicker feature #590

Closed
wants to merge 11 commits into from

Conversation

mustafapsd
Copy link

Hi! I've started to work on date range picker feature #54. I'm a bit confused, I hope I'm doing correct. Features to be added:

  1. Hover indication of the range.
  2. Max & MIn range validations.
  3. Preview format.
  4. inline mode.
  5. Multiple ranges (advanced feature - not mandatory).

import {IDatePickerConfig, IDatePickerConfigInternal} from './date-picker-config.model';
import {IDpDayPickerApi} from './date-picker.api';
import {DatePickerService} from './date-picker.service';
import { IDate, IDateRange } from '../common/models/date.model';
Copy link
Owner

Choose a reason for hiding this comment

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

please revet those changes as they are not relevant

@@ -10,8 +10,8 @@
],
"parserOptions": {
"project": [
"projects/ng2-datepicker/tsconfig.lib.json",
"projects/ng2-datepicker/tsconfig.spec.json"
"projects/ng2-date-picker/tsconfig.lib.json",
Copy link
Owner

Choose a reason for hiding this comment

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

how is that relevant to your changes? I think that you pulled from older version and not from the one in the master

Copy link
Author

Choose a reason for hiding this comment

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

Imports are wrong, so the editor gives errors. You can see these wrong imports.

https://github.com/vlio20/angular-datepicker/blob/master/projects/ng2-date-picker/.eslintrc.json

ControlValueAccessor,
Validator,
OnDestroy {
OnInit,
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert all linting changes.

Copy link
Author

Choose a reason for hiding this comment

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

Opps, sorry

@@ -93,13 +93,16 @@ export class DatePickerComponent implements OnChanges,
@Input() maxDate: SingleCalendarValue;
@Input() minTime: SingleCalendarValue;
@Input() maxTime: SingleCalendarValue;
@Input() dateRangePicker: boolean = false;
@Input() dateRange: IDateRange;
Copy link
Owner

Choose a reason for hiding this comment

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

The range value should be set through the controller input - not as an additional Input of the component.

Copy link
Author

@mustafapsd mustafapsd Feb 15, 2022

Choose a reason for hiding this comment

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

I thought that user can set default values. This is not work now. Should I remove this?

}

this.inputElementValue = (this.utilsService.convertToString(range.from, this.componentConfig.format))
+ ' <> ' + (this.utilsService.convertToString(range.to, this.componentConfig.format));
Copy link
Owner

Choose a reason for hiding this comment

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

  1. Please use template strings
  2. Let's make the delimiter to be configured

Copy link
Author

Choose a reason for hiding this comment

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

  1. Ok
  2. I'll work for limiters tomorrow.

@@ -43,12 +43,32 @@
color: @c-white;
}

.dp-prev-month, .dp-next-month {
.dp-start-date {
background: @c-primary !important;
Copy link
Owner

Choose a reason for hiding this comment

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

please avoid using important. add additional classes if needed.

Copy link
Author

Choose a reason for hiding this comment

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

I've removed, thanks


this.dateRange.to = day.date;
this.onDateRangeSelect.emit(this.dateRange);
// eslint-disable-next-line no-console
Copy link
Owner

Choose a reason for hiding this comment

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

please remove

Copy link
Author

Choose a reason for hiding this comment

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

What's wrong here? Thank you for review.

Copy link
Author

Choose a reason for hiding this comment

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

Oh okey, you want to remove eslint-disable line. I currently using this line for testing.

@mustafapsd
Copy link
Author

I will work tomorrow on the features that you wrote. I did the changes that you mentioned in the review. Thank you for your review, this is really important for me! Could you share a formatting config file (like .prettierrc)?

@vlio20
Copy link
Owner

vlio20 commented Feb 16, 2022

@mustafapsd I don't have one. I am ok with adding a prettier but it should be on a different PR.

@mustafapsd
Copy link
Author

I added hover effect and inline mode.

Video

@vlio20
Copy link
Owner

vlio20 commented Feb 17, 2022

@mustafapsd it's very hard to follow the changes because of the linters. Can you please undo them so I can review the actual code that is related to the range feature?

Few more notes:

  1. Maybe it is better to have a dedicated component for the range picker?
  2. Other calendars should also be supported - dateTime/month.

We would also need to add e2e tests.

@mustafapsd
Copy link
Author

mustafapsd commented Feb 17, 2022

Sorry for linters, writing without formatting is really hard :S

Actually I think the same thing. This will be getting bigger day to day, so will be hard to change.

I never saw time or month pickers in date range pickers. Is it really necessary?

Yes I'll add tests.

@vlio20
Copy link
Owner

vlio20 commented Feb 17, 2022

It is not mandatory but should somehow be reflected there.

For liniting, I suggest that you create a separate PR with the your prettier config file then create PR for the rangepicker. WDYT?

@mustafapsd
Copy link
Author

I removed linting changes for now. Is this logic true for date range pickers? If I'm doing it correctly then I'll create a new component for the range picker.

@vlio20
Copy link
Owner

vlio20 commented Feb 18, 2022

I will try to pull your fork and play with it and give you my feedback

@mustafapsd
Copy link
Author

Thank you!

@vlio20 vlio20 closed this Nov 7, 2024
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.

2 participants