Conversation
7167d25 to
b06d123
Compare
|
| const { isOptionContentAppliedToInput, size = 'medium' } = this.props | ||
| const iconSize = selectSizeToIconSize[size] | ||
| const checkIcon = |
There was a problem hiding this comment.
Unfortunately in my Select rework, some draft code was left in about implementing a check icon on selected items (since then we aggreed not to implement that), so I reverted these here.
4fae7f8 to
a84f237
Compare
| cy.wrap(onChange) | ||
| .should('have.been.called') | ||
| .then((spy) => { | ||
| const selectedDateId: string = spy.lastCall.args[1] |
There was a problem hiding this comment.
In DateInput v2 no id is set on day buttons, so I get the ISO date from the onChange spy.
| cy.get('input[id^="TextInput_"]').as('dateInput') | ||
| cy.get('body').should('contain', errorMsg) | ||
| cy.get('@dateInput').clear().type(`05/18/2017{enter}`) | ||
| cy.get('@dateInput').clear().type(`05/18/2017`).blur() |
There was a problem hiding this comment.
DateInput v1 had an Enter key handler. In v2 there's no such handler, so the .blur() is used.
| | Removed prop | Replacement | | ||
| | --------------------- | -------------------------------- | | ||
| | `renderWeekdayLabels` | Built in — no replacement needed | | ||
|
|
There was a problem hiding this comment.
It might be better to use V12ChangelogTable here.
There was a problem hiding this comment.
@git-nandor V12ChangelogTable is only useful for theme variable changes, but not prop and API changes, so I think I'll leave it as it is
| dateRenderLabel="Date" | ||
| timeRenderLabel="Time" | ||
| invalidDateTimeMessage="Invalid date!" | ||
| calendarIcon="Open calendar" prevMonthLabel="Previous month" nextMonthLabel="Next month" |
There was a problem hiding this comment.
The example code would look better on the documentation page if these were broken into new lines.
|
The commit message doesn't include the breaking prop changes. Overall, it looks good to me! I only have a few minor observations, which I've left in the comments. I found some slight differences between v11.6 and v11.7, but these seem to be specific to the v2 sub-components that DateTimeInput now uses:
|
@git-nandor the commit message includes the prop changes a84f237
|
balzss
left a comment
There was a problem hiding this comment.
overall great work, left some comments about making the api more similar to DateInput
|
|
||
| import { Component, SyntheticEvent } from 'react' | ||
| import { Locale, DateTime, ApplyLocaleContext } from '@instructure/ui-i18n' | ||
| import type { Moment } from '@instructure/ui-i18n' |
There was a problem hiding this comment.
we should not use moment at all
There was a problem hiding this comment.
I looked at this and it would require some major rework and new functions to active the same functionalty. So we agreed to do this in another ticket.
| */ | ||
|
|
||
| import { Component, SyntheticEvent } from 'react' | ||
| import { Locale, DateTime, ApplyLocaleContext } from '@instructure/ui-i18n' |
There was a problem hiding this comment.
we should not use DateTime at all since it's just a moment wrapper
There was a problem hiding this comment.
I looked at this and it would require some major rework and new functions to active the same functionalty. So we agreed to do this in another ticket.
| onRequestValidateDate={this.handleDateValidated} | ||
| onBlur={(e) => this.handleBlur(e)} | ||
| inputRef={dateInputRef} | ||
| placeholder={datePlaceholder ?? ''} |
There was a problem hiding this comment.
this should be just placeholder={datePlaceholder} because DateInput accepts null as a placeholder an uses it's internal placeholder
| locale={locale} | ||
| timezone={timezone} | ||
| disabledDates={disabledDates} | ||
| dateFormat={{ |
There was a problem hiding this comment.
instead of providing this premade formatter and parser, let's allow the users to pass their own or just rely on the one provided by DateInput by default (so dateFormat={props.dateFormat})
There was a problem hiding this comment.
Done. This also changed the default dateFormat Moment's 'LL' (long month name) → locale's default date format, so I had to update the tests accordingly. (e. g. 'May 1, 2017' to '3/29/2022')
| import type { Moment } from '@instructure/ui-i18n' | ||
| import type { Renderable } from '@instructure/shared-types' | ||
|
|
||
| type DateTimeInputProps = { |
There was a problem hiding this comment.
we should make the api of DateTimeInput more similar to DateInput, e.g. instead of separate prevMonthLabel and nextMonthLabel props, just use screenreaderlabel={...
| * If omitted, it will use 'LL' which is a localized date with full month, | ||
| * e.g. "August 6, 2014" | ||
| **/ | ||
| dateFormat?: string |
There was a problem hiding this comment.
this prop should be replaced with the dateFormat type accepted by DateInput
3df60b9 to
0c00547
Compare
| private formatDateInput(date: Date): string { | ||
| const { dateFormat } = this.props |
There was a problem hiding this comment.
I added this which is the same as the formatDate function in DateInput v2. DateTimeInput controls the DateInput value prop directly via state, so it needs to format the date itself.
…DateInput v1 with DateInput v2 in DateTimeInput
BREAKING CHANGE: prevMonthLabel prop removed (use screenReaderLabels.prevMonthButton instead)
nextMonthLabel prop removed (use screenReaderLabels.nextMonthButton instead)
renderWeekdayLabels prop removed
dateFormat type changed: string → string
{ parser: (input: string) => Date
null, formatter: (date: Date) => string }
screenReaderLabels is a new required prop
dateFormat default changed: Moment's 'LL' (long month name) → locale's default date format
INSTUI-4791
0c00547 to
bf24b70
Compare
adamlobler
left a comment
There was a problem hiding this comment.
I checked the components and other than the yearPicker issue I mentioned in the channel, I think it’s okay. I created a ticket for that earlier: https://instructure.atlassian.net/browse/INSTUI-5014
BREAKING CHANGE:
INSTUI-4991
ISSUE:
TEST PLAN: