make event colors optional #473

etwillbefine · 2018-03-08T19:24:39Z

relates to #468

etwillbefine · 2018-03-08T19:27:07Z

Not sure about the defaults.scss. Couldnt find any other suitable place.

ejs was missing for me. Installed the module but now the package-lock went crazy. Shall I remove the commit again?

Do you have any thoughts on how to treat this line:
https://github.com/mattlewis92/angular-calendar/blob/master/src/modules/month/calendar-month-view.component.ts#L315

Not sure if we just go with (event.color && event.color.secondary) || 'default') ?!

mattlewis92

Sorry about the late reply on this one, am just getting ready to move country at the moment and things are a bit hectic while I sort things out 😄

re https://github.com/mattlewis92/angular-calendar/blob/master/src/modules/month/calendar-month-view.component.ts#L315 I would just set it to #005cbf by default (same logic as is in the CSS)

mattlewis92 · 2018-03-23T19:44:05Z

src/defaults.scss

@@ -0,0 +1,6 @@
+
+.cal-event {


Rather than having defaults.scss I think it's better to just have the CSS copied under each of the views, it also prevents adding the global cal-event rule to the whole app

Also for the month view the background color is the primary color, and for the day / week views its the secondary color, and the border color is the primary one

mattlewis92 · 2018-03-23T19:44:25Z

src/defaults.scss

+.cal-event {
+  background-color: #0062cc;
+  border-color: #005cbf;
+  color: #fff;


How come this is needed?

If there is no color specified it will use the font set on a parent. It may doesn't fit to the background and needs extra Styling. That was my thought.

mattlewis92 · 2018-03-23T19:45:29Z

package.json

@@ -124,6 +124,7 @@
    "core-js": "^2.5.1",
    "create-plunker": "^1.4.0",
    "css-loader": "^0.28.9",
+    "ejs": "^2.5.7",


This shouldn't be needed - are you running the latest version of npm? If you copy the package-lock.json from master and rm -rf node_modules first before doing npm i it should do the trick.

I checked out the project from scratch and did npm I. When running the tests it complained that ejs was missing. I'll give it another try and update my npm to latest.

mattlewis92 · 2018-03-23T19:46:18Z

test/calendar-week-view.component.spec.ts

+      fixture.nativeElement.querySelector('.cal-event')
+    );
+    expect(computedStyles.getPropertyValue('background-color')).to.equal(
+      'rgb(0, 98, 204)'


That's a clever way of testing this change, nice idea! 😄

use correct color scheme for views use blue scheme from existing demos apply fallback color if none specified when toggling daylight remove default.scss apply currentColor to links within cal-event-title

etwillbefine · 2018-03-25T14:09:48Z

src/modules/day/calendar-day-view.scss

+  }
+
+  .cal-event-title:link {
+    color: currentColor;


sorry for this additional change. I can also remove it again but it was strange to see dots from ellipsis in the specified color and the link in blue (bootstrap) in the demos..

I learnt something new today from this, thanks! 😄

etwillbefine · 2018-03-25T14:12:57Z

I changed the colors to use the existing blue scheme from the demos. Bootstrap was using grey as secondary color... Hope that this addresses all your concerns.

codecov-io · 2018-03-27T11:56:12Z

Codecov Report

Merging #473 into master will decrease coverage by 0.16%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #473      +/-   ##
==========================================
- Coverage   96.82%   96.66%   -0.17%     
==========================================
  Files          34       34              
  Lines         599      600       +1     
  Branches       61       62       +1     
==========================================
  Hits          580      580              
  Misses          1        1              
- Partials       18       19       +1

Impacted Files	Coverage Δ
...rc/modules/day/calendar-all-day-event.component.ts	`100% <ø> (ø)`	⬆️
...odules/month/calendar-open-day-events.component.ts	`100% <ø> (ø)`	⬆️
...modules/week/calendar-week-view-event.component.ts	`100% <ø> (ø)`	⬆️
src/modules/month/calendar-month-cell.component.ts	`100% <ø> (ø)`	⬆️
...c/modules/day/calendar-day-view-event.component.ts	`100% <ø> (ø)`	⬆️
src/modules/month/calendar-month-view.component.ts	`93.02% <50%> (-1.1%)`	⬇️

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 f76641a...cc6562e. Read the comment docs.

mattlewis92

I think the week view component template needs to override the border color with the primary one (if set). Currently the week view events don't have borders (I think they should for consistency with the day view)

On the month view, I think the event titles are more readable in white, you still have the colored dot so you know the color. WDYT?

Master:

This PR

etwillbefine · 2018-03-28T10:17:25Z

Thanks for your hints! The event-title color is already white, it was the link color again.
Imo a central place for common styles is missing. Needed to duplicate some code now (for the link color) ..

Border-color on the week view will now update according to the color specified in the event.
I'm on vacation from tonight. Might take a little longer to respond.

mattlewis92 · 2018-03-28T16:50:47Z

Perfect, thanks a lot Tim! Have an awesome easter weekend! 😄

mattlewis92 · 2018-04-17T10:47:42Z

Whoops, sorry I completely forgot to release this, done now in 0.24.0 😄

etwillbefine force-pushed the optional-event-colors branch from 43c9c44 to 55fbebc Compare March 8, 2018 19:26

mattlewis92 requested changes Mar 23, 2018

View reviewed changes

etwillbefine force-pushed the optional-event-colors branch 2 times, most recently from 6095060 to ba0ab58 Compare March 25, 2018 14:06

etwillbefine added 2 commits March 25, 2018 16:06

feat: make event colors optional and apply default

136f551

chore: updates regarding review

5027de3

use correct color scheme for views use blue scheme from existing demos apply fallback color if none specified when toggling daylight remove default.scss apply currentColor to links within cal-event-title

etwillbefine force-pushed the optional-event-colors branch from ba0ab58 to 5027de3 Compare March 25, 2018 14:07

etwillbefine commented Mar 25, 2018

View reviewed changes

chore: upgrade calendar-utils

a41036b

mattlewis92 requested changes Mar 27, 2018

View reviewed changes

feat: override border color on week view, set link color on month view

cc6562e

mattlewis92 approved these changes Mar 28, 2018

View reviewed changes

mattlewis92 merged commit 4f9ed24 into master Mar 28, 2018

mattlewis92 deleted the optional-event-colors branch March 28, 2018 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make event colors optional #473

make event colors optional #473

etwillbefine commented Mar 8, 2018

etwillbefine commented Mar 8, 2018 •

edited

Loading

mattlewis92 left a comment •

edited

Loading

mattlewis92 Mar 23, 2018

mattlewis92 Mar 23, 2018

mattlewis92 Mar 23, 2018

etwillbefine Mar 24, 2018

mattlewis92 Mar 23, 2018

etwillbefine Mar 24, 2018

mattlewis92 Mar 23, 2018

etwillbefine Mar 25, 2018

mattlewis92 Mar 27, 2018

etwillbefine commented Mar 25, 2018

codecov-io commented Mar 27, 2018 •

edited

Loading

mattlewis92 left a comment

etwillbefine commented Mar 28, 2018

mattlewis92 commented Mar 28, 2018

mattlewis92 commented Apr 17, 2018

make event colors optional #473

make event colors optional #473

Conversation

etwillbefine commented Mar 8, 2018

etwillbefine commented Mar 8, 2018 • edited Loading

mattlewis92 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etwillbefine commented Mar 25, 2018

codecov-io commented Mar 27, 2018 • edited Loading

Codecov Report

mattlewis92 left a comment

Choose a reason for hiding this comment

etwillbefine commented Mar 28, 2018

mattlewis92 commented Mar 28, 2018

mattlewis92 commented Apr 17, 2018

etwillbefine commented Mar 8, 2018 •

edited

Loading

mattlewis92 left a comment •

edited

Loading

codecov-io commented Mar 27, 2018 •

edited

Loading