Implement an equivalent of org-agenda-start-on-weekday #266 #676
Conversation
- Stop using null to mean org-agenda-start-on-weekday = nil because it is very confusing. -1 was chosen instead because it is a number. - On mobile, three-letter week names are too long. Now the week names only get one character each.
| return { | ||
| files: determineIncludedFiles(allFiles, fileSettings, path, 'includeInAgenda', false), | ||
| todoKeywordSets: file.get('todoKeywordSets'), | ||
| agendaTimeframe: state.base.get('agendaTimeframe'), | ||
| agendaDefaultDeadlineDelayValue: state.base.get('agendaDefaultDeadlineDelayValue') || 5, | ||
| agendaDefaultDeadlineDelayUnit: state.base.get('agendaDefaultDeadlineDelayUnit') || 'd', | ||
| agendaStartOnWeekday: agendaStartOnWeekday == null ? 1 : +agendaStartOnWeekday, |
There was a problem hiding this comment.
Why not ===? I thought it's best practice to use it instead of ==. Why the +? To convert to integer?
How far do we trust the state store in terms of data integrity? The existing lines above this one don't check the types. So I think we better stick to that scheme. @munen what do you think?
There was a problem hiding this comment.
Right, triple equals is best practice in general but some consider double equals against null and undefined to be an exception since it’s a convenient way of eliminating both at once. Yes + is for number conversion (maybe this was too hacky)
I’m type checking here because falseness check interferes with 0... I’ll comment more on that in #677
| return { | ||
| fontSize: state.base.get('fontSize') || 'Regular', | ||
| bulletStyle: state.base.get('bulletStyle'), | ||
| shouldTapTodoToAdvance: state.base.get('shouldTapTodoToAdvance'), | ||
| agendaDefaultDeadlineDelayValue: state.base.get('agendaDefaultDeadlineDelayValue') || 5, | ||
| agendaDefaultDeadlineDelayUnit: state.base.get('agendaDefaultDeadlineDelayUnit') || 'd', | ||
| agendaStartOnWeekday: agendaStartOnWeekday == null ? 1 : +agendaStartOnWeekday, |
There was a problem hiding this comment.
Same question as above, here.
|
Hi @tomonacci Thank you for your contribution 🙏 @schoettl Thank you for the code review 🙏 I've looked through the code. It works well, but I'd make some changes. I'll pull PR into the I haven't written a test, yet. If you can do so, as @schoettl suggested, that would be nice^^ |
WISOTT. One caveat is that I changed the default start of the week to Monday from Sunday to align with the default in Org to minimize surprise for people coming from Org (at the expense of surprising longtime Organice users).
Controls in the settings:
