Removing date segment placeholder default width #6562
Conversation
ktabors
changed the title
ktabors
changed the title
|
One other problem appears to be that when you type into the field, the entire field gets wider or shorter... |
ktabors
changed the title
ktabors
changed the title
|
|
||
| // The max of two is for times with only hours. | ||
| // As the length of a date grows we need to proportionally increase the width. | ||
| // We use the characts with 'ch' and months and time dashes are wider and this |
There was a problem hiding this comment.
| // We use the characts with 'ch' and months and time dashes are wider and this | |
| // We use the characters width 'ch' and months and time dashes are wider and this |
?? this sentence didn't make much sense
I assume this is trying to explain why we do a divide by 5? i don't really follow why that is. Maybe some examples would help?
There was a problem hiding this comment.
I don't quite follow the calculation either, I think I've formatted as wide of a date as possible but the min width still gives some extra padding here
The character count is 24 and the calculated character width with the additional accommodation factor sets the width to 28ch which gives it that extra purple padding. Using 24ch makes it pretty flush with the inputted date
which seems better?
There was a problem hiding this comment.
I think the problem is when you change it to a placeholder. "mm" is much wider than two numbers, so it would push beyond the min-width causing the field to grow.
There was a problem hiding this comment.
still seems to have a bit of extra space by ~1 or 2ch:
I'm fine with it being a bit off/not exactly precise, just wanna understand the underlying calculation here to see if there is any way we can make it more precise, the white space after the field is already pretty large compared to before (of which people already complained about haha)
There was a problem hiding this comment.
TLDR: This works remarkably well as a guess/approximation and is because of the width of month and dash characters in placeholder values.
It does widen a few cases. I tried a bunch of things with ch and mm units, including a granularity-based switch statement. This ratio works well, and it is remarkably close. Since we're guessing and using today's date, not the placeholder or the actual date value, I'm not sure if getting more precise would be better.
When Devon and I were looking at it and talking about it yesterday, it ended up being that we need approximately two characters for any data value less than 10 characters. For example, we have a problem with TimeField width shifting for granularity hour if we don't give it four characters of space. Like Devon stated above, for dates it's the months and for time's it's the dashes that create the need for extra characters. As we grow past ten characters, we're adding time which is dashes so we need more characters.
Also, this seems to work with locales, but we don't have a collection of rigorous tests with all the possible font, language, and calendar combinations. Getting more precise for English might cause problems elsewhere, while being more generous has worked so for with the locale testing that has been done.
Yes, it looks "bad" when you have a DateRange with granularity seconds with two values, where it only fills about half the field, but when you switch it to placeholder it fills it back up. We're shifting from all the white space being in the segments to the white space being after the date values. The field widths didn't change much. Looking at chromatic most of the field widths didn't grow in width and it was only by a little for those that did. It was about equivalent width grown to the purple space in that image Daniel shared.
There was a problem hiding this comment.
Instead of guesstimating, could we make use of useLayoutEffect to render the placeholder, measure, then replace the placeholder with the real value before paint actually happens? Then we should be exact.
We could also render it position absolute and visibility hidden behind the real one, that way we don't need to make a change to state and it'd pick up all the correct CSS inheritance
There was a problem hiding this comment.
That was my first approach. I recall there being edge cases, it broke a lot of tests even with aria-hidden, and we still have the DateRangePicker problem of needing to set the value higher up than we have the value, placeholder, and segments.
| @@ -52,6 +52,8 @@ function TimeField<T extends TimeValue>(props: SpectrumTimeFieldProps<T>, ref: F | |||
|
|
|||
| let validationState = state.validationState || (isInvalid ? 'invalid' : null); | |||
|
|
|||
| let characterCount = useDateCharacterWidth(state) + 'ch'; | |||
There was a problem hiding this comment.
this isn't actually a character count though is it? nor a character width?
There was a problem hiding this comment.
Both... It's the character count being turned into a width. And on Picker it's a calc() which makes it more width.
| @@ -77,3 +77,16 @@ export function useFocusManagerRef(ref: FocusableRef<HTMLElement>) { | |||
| })); | |||
| return domRef; | |||
| } | |||
|
|
|||
| export function useDateCharacterWidth(state) { | |||
There was a problem hiding this comment.
is it actually a character width? or is it the width of the characters + some fudging number? is this actually useDateSegmentWidth?
There was a problem hiding this comment.
It's more like useApproximateFormattedDateWidth, but that is too long. I'll update it along with characterCount to something better.
There was a problem hiding this comment.
I'm fine with the hook name being long, it's private and i'd prefer the name be accurate
There was a problem hiding this comment.
Feels like a lot of whitespace on the end but I don't think there is much we can do about that without making the field width shift which feels worse haha.
I also found that Android now crashes when trying to change AM/PM in the time field. Steps to reproduce:
- Go to https://reactspectrum.blob.core.windows.net/reactspectrum/1a948f002e8a7279c153110cbe0b9d4f4e8f5820/storybook/index.html?path=/story/date-and-time-timefield--default&providerSwitcher-express=false
- Fill out the field with any time
- Focus the AM/PM field and hit backspace to clear it. Focus should move on to the previous segment
- tap the AM/PM segment and try to change it to PM by pressing P. Note it doesn't change.
- Press P again and note it crashes
|
|
||
| // The max of two is for times with only hours. | ||
| // As the length of a date grows we need to proportionally increase the width. | ||
| // We use the characts with 'ch' and months and time dashes are wider and this |
There was a problem hiding this comment.
I don't quite follow the calculation either, I think I've formatted as wide of a date as possible but the min width still gives some extra padding here
The character count is 24 and the calculated character width with the additional accommodation factor sets the width to 28ch which gives it that extra purple padding. Using 24ch makes it pretty flush with the inputted date
which seems better?
Co-authored-by: Robert Snow <[email protected]>
I was able to reproduce it on an another PR build. Follow the same steps, but when it doesn't crash press backspace and then it crashes. We should enter a bug for this. |
There was a problem hiding this comment.
I assume we're ok with the minWidth on all these fields changing in response to the time? Especially for ones with shorter durations, like fields containing seconds. Or even just moving from AM -> PM I think was noted? So theoretically the minWidth could change mid session.
Sounds like it might cause problems for chromatic unless we mock the time for those?
|
## API Changes
unknown top level export { type: 'any' } @react-stately/datepickerDateFieldState DateFieldState {
calendar: Calendar
clearSegment: (SegmentType) => void
confirmPlaceholder: () => void
dateFormatter: DateFormatter
dateValue: Date
decrement: (SegmentType) => void
decrementPage: (SegmentType) => void
formatValue: (FieldOptions) => string
+ getDateFormatter: (string, FormatterOptions) => DateFormatter
granularity: Granularity
increment: (SegmentType) => void
incrementPage: (SegmentType) => void
isDisabled: boolean
isReadOnly: boolean
isRequired: boolean
maxGranularity: 'year' | 'month' | Granularity
segments: Array<DateSegment>
setSegment: (SegmentType, number) => void
setValue: (DateValue) => void
value: DateValue
}
it changed:
DatePickerState DatePickerState {
dateValue: DateValue
formatValue: (string, FieldOptions) => string
+ getDateFormatter: (string, FormatterOptions) => DateFormatter
granularity: Granularity
hasTime: boolean
isInvalid: boolean
isOpen: boolean
setOpen: (boolean) => void
setTimeValue: (TimeValue) => void
setValue: (DateValue | null) => void
timeValue: TimeValue
value: DateValue | null
}
it changed:
DateRangePickerState DateRangePickerState {
dateRange: DateRange | null
formatValue: (string, FieldOptions) => {
start: string
end: string
}
+ getDateFormatter: (string, FormatterOptions) => DateFormatter
granularity: Granularity
hasTime: boolean
isInvalid: boolean
isOpen: boolean
setDateRange: (DateRange) => void
setDateTime: ('start' | 'end', DateValue) => void
setOpen: (boolean) => void
setTime: ('start' | 'end', TimeValue) => void
setTimeRange: (TimeRange) => void
setValue: (DateRange | null) => void
timeRange: TimeRange | null
value: DateRange | null
}
it changed:
|
There was a problem hiding this comment.
Approving for testing, filed https://github.com/orgs/adobe/projects/19/views/4?pane=issue&itemId=70985842 for the Android crash since it is pre-existing.
Seems like this bug is basically #6183 except that instead of crashing, it concatenates AM and PM (so that it looks like AMPM). I followed the same steps to reproduce the crash above on main, and when it would crash here, it would concatenate AMPM. |
Full width is no longer reserved for date segments in DateField, TimeField, DatePicker, and DateRangePicker. Segments take the space of the placeholder or their value.
This also resolves #6183
Field sizes might adjust because the placeholder is no longer italic which makes it a few pixels smaller. This will break a lot of Chromatic stories.
There are some existing component fields that already shrink/grow when you edit the segments. It's only with some components, in some cases. This can be caused by:
🧢 Your Project:
RSP