Add a customizable, slottable day component for calendars #745
Conversation
|
Demo will be published at https://apps.inindca.com/common-ui-docs/genesys-webcomponents/feature/calendar-custom-date |
github-actions
bot
requested review from
daragh-king-genesys and
gavin-everett-genesys
| if (target) { | ||
| target.focus(); | ||
| } | ||
| } | ||
|
|
||
| private onKeyDown(event: KeyboardEvent): void { | ||
| console.log('KEYDOWN', event.key); |
There was a problem hiding this comment.
minor this can be removed
There was a problem hiding this comment.
I will also look into why linting didn't catch this
| {week.dates.map(day => { | ||
| const isoDateStr = asIsoDate(day.date); | ||
| return ( | ||
| <slot key={isoDateStr} name={isoDateStr}> |
There was a problem hiding this comment.
Is the key property valid on the slot element ?
There was a problem hiding this comment.
I think it should be, it's a JSX/Stencil feature, not a native one: https://stenciljs.com/docs/templating-jsx#dealing-with-children. We don't do a lot of removing/rearranging dates in the calendar, but I try to always add a key when creating children in a loop as a general best practice.
| return ( | ||
| <slot key={isoDateStr} name={isoDateStr}> | ||
| <gux-day | ||
| day={isoDateStr} |
There was a problem hiding this comment.
I am not sure if the day property will work for example if you have a gux-day component like
<gux-day slot="2024-05-05" day="2024-05-10"></gux-day the calendar will be out of sync. I am not sure what the best way to handle that scenario would be. My suggestion would be either a single source of truth like keeping the slot attribute and somehow accessing the value in gux-day through an attribute on the parent maybe but I am not sure how that would work with how the calendar is rendered. My other sugguestion would be to just log a warning.
| font-size: var(--gse-ui-calendarMenu-date-defaultText-fontSize); | ||
| font-style: normal; | ||
| font-weight: var(--gse-ui-calendarMenu-date-defaultText-fontWeight); | ||
| line-height: 32px; |
There was a problem hiding this comment.
Token needed here
|
|
||
| @Component({ | ||
| styleUrl: 'gux-day.scss', | ||
| tag: 'gux-day', |
There was a problem hiding this comment.
i think this should have -beta prefix
| export class GuxElement { | ||
| @Element() | ||
| root: HTMLElement; | ||
| } |
There was a problem hiding this comment.
Think this can be removed
There was a problem hiding this comment.
Ah yeah, that was an experiment that didn't get cleaned up. Thanks for catching it.
| this.date = new Date(this.day); | ||
| const locale = getDesiredLocale(this.root); | ||
| this.dayFormatter = sparkIntl.dateTimeFormat(locale, { day: 'numeric' }); | ||
| this.readerFormatter = sparkIntl.dateTimeFormat(locale); |
There was a problem hiding this comment.
this is not used
| /* Watcher to sync the internal date with the day attribute on changes */ | ||
| @Watch('day') | ||
| onDateAttrChange(): void { | ||
| this.date = new Date(this.day); |
There was a problem hiding this comment.
@MattCheely welcome to developing with Javascript Dates in the western hemisphere. This does not work as expected for timezones behind GMT.
The calendar for someone in the US for example will be shifted a day and be incorrect
| aria-current={day.selected ? 'true' : 'false'} | ||
| tabindex={day.selected || day.focused ? '0' : '-1'} | ||
| onKeyDown={e => void this.onKeyDown(e)} | ||
| aria-disabled={day.disabled ? 'true' : 'false'} |
There was a problem hiding this comment.
If a developer slots their own gux-day they will be responsible for making sure aria-current, aria-disabled and tabindex are correct right?
|
API looks good to me :) , Just one minor visual item,
|
No description provided.