Conversation
|
any thoughts? would love to get this into master |
|
Sorry, I forgot about this. Reviewing |
cibernox
left a comment
cibernox
left a comment
There was a problem hiding this comment.
The functionality seems to work but it also seems to unrelated changes.
| let i = 0; | ||
| while (days[i]) { | ||
| let daysOfWeek = days.slice(i, i + 7); | ||
| if (!showDaysAround) { |
There was a problem hiding this comment.
handled in template instead; allows for better screen layout in the false state of this condition
| number: momentDate.date(), | ||
| date: momentDate._d, | ||
| moment: momentDate, | ||
| dow: momentDate.format('ddd').toLowerCase(), |
There was a problem hiding this comment.
I'd prefer dow to be more clear even if that means it has to be more verbose. dayName?
There was a problem hiding this comment.
i've seen "dow" refer to day-of-week a lot but I guess it may not jump out at many people. I'll change that today but one thought is that maybe we should call dayAbbrev or dayCode as this may help to distinguish it from the more user-oriented uppercased name.
| isFocused: this.get('focusedId') === id, | ||
| isCurrentMonth: momentDate.month() === calendar.center.month(), | ||
| isCurrentMonth, | ||
| isVisible: isCurrentMonth || this.get('showDaysAround'), |
| {{day.number}} | ||
| {{/if}} | ||
| </button> | ||
| {{#if day.isVisible}} |
There was a problem hiding this comment.
After your changes in the component you need to do this {{if}} for every day in the month. Why do this? It seems unrelated with the overall goal of the PR, which is add data-day-of-week="{{day.dow}}" to the days.
There was a problem hiding this comment.
it ensures that each day -- whether visible or not -- has a ember-power-calendar-day block in it. This makes the CSS styling easier and allows you to take fuller advantage of flexbox.
There was a problem hiding this comment.
days which are "not visible" are easily targeted via the .ember-power-calendar-day--blank selector
package.json
Outdated
| "loader.js": "^4.0.10", | ||
| "memory-scroll": "0.2.0" | ||
| "memory-scroll": "0.2.0", | ||
| "phantomjs-prebuilt": "2" |
There was a problem hiding this comment.
Can you remove phantom? Phantomjs is dead as a project is dead and the addon is tested in chrome.
There was a problem hiding this comment.
yup sure, can't remember why I added it all
| @@ -1,3 +1,6 @@ | |||
| $cell-spacing: 2px; | |||
There was a problem hiding this comment.
All this CSS changes also seem unrelated to the topic of the PR.
There was a problem hiding this comment.
true but it is fixing an existing defect that I messaged to you about ... when days are not visible you start to have layout problems with the old styling
|
what do you need to get this accepted? |
2 participants
the functionality we discussed is working and was quite a simple addition but due to my poor testing skills somehow the two tests I added fail. I've commented them out for the time being to ensure nothing was broken but if you have a look I suspect you'll see what was wrong with my tests quickly.