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(datepicker): input datepicker global config #3275

Merged

Conversation

IAfanasov
Copy link
Contributor

@IAfanasov IAfanasov commented Jul 7, 2019

Closes #3273

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md guide.
  • built and tested the changes locally.
  • added/updated any applicable tests.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.

datepicker

@codecov-io
Copy link

codecov-io commented Jul 7, 2019

Codecov Report

Merging #3275 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3275      +/-   ##
==========================================
+ Coverage   91.13%   91.14%   +0.01%     
==========================================
  Files          95       96       +1     
  Lines        2796     2800       +4     
  Branches      516      516              
==========================================
+ Hits         2548     2552       +4     
  Misses        190      190              
  Partials       58       58
Flag Coverage Δ
#e2e 56.21% <100%> (+0.06%) ⬆️
#unit 87.99% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
src/index.ts 100% <ø> (ø) ⬆️
src/datepicker/datepicker.module.ts 100% <ø> (ø) ⬆️
src/datepicker/datepicker-input-config.ts 100% <100%> (ø)
src/datepicker/datepicker-input.ts 96.4% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b35f759...fbd6e8d. Read the comment docs.

@benouat
Copy link
Member

benouat commented Jul 15, 2019

Hey @IAfanasov! Sorry for the late review start.

Even before I start the review, I think I have one overall point to raise
I agree that for us to tackle #3273 we need to introduce a new config file. ngb-datepicker is not only 1 single widget, but more 2 widgets: Calendar and InputDatepicker.

So this placement config definitely belongs to a new NgbInputDatepickerConfig (like you did in this PR)

Nonetheless I think it would make sense for that NgbInputDatepickerConfig inherits from NgbDatepickerConfig no ? Or if inheritance does not make sense (usually extends does not fit very well with DI) we could at least have NgbInputDatepickerConfig to inject NgbDatepickerConfig and have both of them implements a common interface.

I don't really like that internally we have to inject both config inside the constructor of NgbIputDatepicjer.
On top, I think that people might want to be able to override config for calendar and inputdatepicker in a kind of independent way.

It's just a quick thought on the topic, I a not really sure nor sold about it.

WDYT ? cc @pkozlowski-opensource

@IAfanasov
Copy link
Contributor Author

IAfanasov commented Jul 15, 2019

Hey @IAfanasov! Sorry for the late review start.

Hi @benouat! No worries for late review (: Glad to receive review and make valuable contribution! (:

Nonetheless I think it would make sense for that NgbInputDatepickerConfig inherits from NgbDatepickerConfig no ? Or if inheritance does not make sense (usually extends does not fit very well with DI) we could at least have NgbInputDatepickerConfig to inject NgbDatepickerConfig and have both of them implements a common interface.

I don't really like that internally we have to inject both config inside the constructor of NgbIputDatepicjer.
On top, I think that people might want to be able to override config for calendar and inputdatepicker in a kind of independent way.

Don't see clear solution here as well (: My impression - NgbDatepickerInput reuses NgbDatepicker. It's not one more NgbDatepicker that should be anyhow different from other NgbDatepickers in the app. Thats why I see configs for this components as 2 independent instances without any relations between each other. It's only my opinion. Totally open for suggestions (:

To prevent injections of 2 services injection we can for sure inject config of NgbDatepickerConfig into NgbInputDatepickerConfig. When developer would use it as

constructor(inputDatepickerConfig: NgbInputDatepickerConfig) {
    // 1. approach with getters and setters for injected into `NgbInputDatepickerConfig` `datepickerConfig`
    inputDatepickerConfig.minDate = {year: 1900, month: 1, day: 1};
    // 2. approach with access to `datepickerConfig`
    inputDatepickerConfig.datepickerConfig.minDate = {year: 1900, month: 1, day: 1};
}

Feel like option 2 reveals to developer nature of configs right away while 1 can fool developer into thinking it's independent config.

@IAfanasovMob
Copy link

Hey @benouat ! (:
seems like @pkozlowski-opensource doesn't have time for this.
is there other option to make a decision and do this functionality? (:

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it's a very welcome change!

My vote also goes for extending, because I simply don't see a case when it shouldn't be a superset.

Also it should be simpler for the user to deal with 1 config, not 2. And we're duplicating all inputs already for better or worse...

@Injectable()
export class NgbInputDatepickerConfig extends NgbDatepickerConfig {
   autoClose: boolean | 'inside' | 'outside' = true;
   container: null | 'body';
   positionTarget: string | HTMLElement;
   placement: PlacementArray = ['bottom-left', 'bottom-right', 'top-left', 'top-right'];
}

@maxokorokov maxokorokov self-assigned this Nov 14, 2019
@maxokorokov maxokorokov added this to the 5.2.0 milestone Nov 14, 2019
@maxokorokov maxokorokov merged commit cd3f6ba into ng-bootstrap:master Nov 15, 2019
@IAfanasov IAfanasov deleted the feature/input-global-config branch December 12, 2019 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Datepicker placement global settings
5 participants