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

fix(datepicker): remove hardcoded bg-light #3351

Conversation

gpolychronis
Copy link
Contributor

@benouat
@maxokorokov

Please review

@@ -20,6 +20,7 @@ ngb-datepicker-month-view {
&-weekdays {
border-bottom: 1px solid rgba(0, 0, 0, 0.125);
border-radius: 0;
background-color: var(--light);
Copy link
Member

Choose a reason for hiding this comment

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

Using 'var' was a good idea, but unfortunately we can't use this functionality as ng-bootstrap supports IE >= 10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

var is already in use:

&-weekday {
color: #5bc0de;
color: var(--info);
}

Do it like this maybe better?

@fbasso
Copy link
Member

fbasso commented Sep 5, 2019

Thanks for your PR @gpolychronisamadeus !

You'll also need to rename the commit message (and the title by the way) with fix(datepicker): ... (instead of chore), and add a comment fix #... to reference the original issue.

@codecov-io
Copy link

codecov-io commented Sep 5, 2019

Codecov Report

Merging #3351 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3351   +/-   ##
=======================================
  Coverage   90.88%   90.88%           
=======================================
  Files          95       95           
  Lines        2743     2743           
  Branches      510      510           
=======================================
  Hits         2493     2493           
  Misses        190      190           
  Partials       60       60
Flag Coverage Δ
#e2e 55.01% <ø> (ø) ⬆️
#unit 87.99% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/datepicker/datepicker-month-view.ts 100% <ø> (ø) ⬆️

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 eb26beb...9cd0e30. Read the comment docs.

@gpolychronis gpolychronis force-pushed the datepicker/remove-hardcoded-style branch from dbeea62 to 9cd0e30 Compare September 5, 2019 13:26
@fbasso fbasso changed the title chore(datepicker): remove hardcoded bg-light fix(datepicker): remove hardcoded bg-light Sep 5, 2019
@fbasso
Copy link
Member

fbasso commented Sep 5, 2019

I just changed the title with fix (less important than the commit message, but just to be coherent). Otherwise, LGTM !

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.

LGTM, thanks!

@maxokorokov maxokorokov added this to the 5.1.2 milestone Sep 6, 2019
@maxokorokov maxokorokov merged commit c7bc4d7 into ng-bootstrap:master Sep 6, 2019
@@ -20,6 +20,8 @@ ngb-datepicker-month-view {
&-weekdays {
border-bottom: 1px solid rgba(0, 0, 0, 0.125);
border-radius: 0;
background-color: #f8f9fa;

Choose a reason for hiding this comment

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

Removing bg-light class but you are also introducing a hard coded background color. Why not set on parent background color and in custom css it can be overridden?

Copy link
Member

Choose a reason for hiding this comment

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

.ngb-datepicker-month-view-weekdays is a custom-class publicly available.
You can override the background-color by defining your own. Isn't it enough ?

Choose a reason for hiding this comment

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

I can but I have to override for each including header, week, weekday, day using !important

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.

None yet

6 participants