feat: adding date description texts #45
Conversation
There was a problem hiding this comment.
First, thanks for the effort. I wonder if this addDateDescription approach is too restrictive. I'm a little afraid that this will only solve a very narrow/specific edge case. A more flexible approach could be to let the user opt out of the standard date rendering completely, and let him specify a renderDate function that returns an HTMLElement to be rendered, instead of the default one. This could easily solve your use case, and also leaves room for many others.
| @Prop() goToRangeStartOnSelect?: boolean = true; | ||
|
|
||
| @Prop({ mutable: true }) showDayDescriptions?: boolean = false; | ||
| @Prop({ mutable: true }) addDateDescription?: (date: Date) => string; |
There was a problem hiding this comment.
issue: I don't think we need the { mutable: true } here. The prop value is not mutated within the component.
| @Prop() maxSearchDays?: number = 365; | ||
| @Prop() goToRangeStartOnSelect?: boolean = true; | ||
|
|
||
| @Prop({ mutable: true }) showDayDescriptions?: boolean = false; |
There was a problem hiding this comment.
question: Do we need this prop? This could be implicit information by looking at addDateDescription,. right? If addDateDescription is not set or returns undefined, the description should not be shown.
| @Prop() goToRangeStartOnSelect?: boolean = true; | ||
|
|
||
| @Prop({ mutable: true }) showDayDescriptions?: boolean = false; | ||
| @Prop({ mutable: true }) addDateDescription?: (date: Date) => string; |
There was a problem hiding this comment.
suggestion: Allow it to return undefined to explicitly skip the descriptions for a date.
| @Method() | ||
| async getDescriptionForDay(date: Date): Promise<string> { | ||
| return this.dayDescriptions[getISODateString(date)] || ''; | ||
| } | ||
|
|
||
| @Method() | ||
| async getAllDayDescriptions(): Promise<Record<string, string>> { | ||
| return { ...this.dayDescriptions }; | ||
| } | ||
|
|
There was a problem hiding this comment.
question: Is there a need to make these methods part of the public component API with the @Method decorator. I don't see an immediate use case why you need to retrieve the descriptions this way.
| } | ||
| } | ||
|
|
||
| private setDescriptionForDay(date: Date): string { |
There was a problem hiding this comment.
question: Do you expect the addDateDescription functions to be expensive to run? Otherwise I would suggest to not cache the values at all and just call the function during render. This would simplify your changes a lot. (No need for the dayDescriptions record, or the new methods.)
| <div class={this.getClassName('date-content')}> | ||
| <Tag aria-hidden="true">{day.getDate()}</Tag> | ||
| {this.showDayDescriptions && dayDescription && ( | ||
| <span class={this.getClassName('date-description')} aria-hidden="true"> |
There was a problem hiding this comment.
issue: The new classnames here should be documented in the index.html.
2 participants
I'm working in a project that needs some description texts for some dates and in this PR I'm sending this change since it could be a common feature in some projects that have texts to display , like discounts or another information, on each field date.
Example