PB-424: Time slider year entry button#782
Conversation
Passing run #1801 ↗︎
Details:
Review all test suite changes for PR #782 ↗︎ |
|||||||||||||||
466ab84 to
dbf8d93
Compare
| <input | ||
| v-model="displayedYear" | ||
| data-cy="time-slider-bar-cursor-year" | ||
| maxlength="4" | ||
| class="time-slider-bar-cursor-year" | ||
| type="text" | ||
| @input="setYearToInputIfValid" | ||
| /> |
There was a problem hiding this comment.
The input should only allow numbers and a valid year between the min an max defined. If an invalid year is provided the input box should be marked as invalid in red using the bootstrap validation (see https://getbootstrap.com/docs/5.0/forms/validation/ and some example in the code).
Pressing enter should validate the year and exit the input editing.
On the year input we should not allow to grab and move the cursor as it make almost impossible to select the year by click and select.
There was a problem hiding this comment.
Is there a specific reason why the year should only be evaluated after pressing enter? I personally prefer if it auto validates after the input contains four characters
ltkum
left a comment
There was a problem hiding this comment.
I was not so sure about the watcher, but in the end, it's a durable approach if we start modifying currentYear in multiple part of the file.
After testing a bit (and checking the legacy viewer), there is something that's not very user friendly : when selecting the input field and keeping the mouse pressed, we both move the time slider and the selection within the input field.
On the old mapviewer, only the sides of the time slider cursor allowed it to move, and the middle was only an input field. I would also keep it like that :
clicking on the input field : does not call the 'onMouseDown' event which allows the time slider to move and to set a year.
clicking on the sides : same behavior as now.
this would also reduce a bit the commits sent by the timeSlider
@ltshb what do you think ? (we can totally do this in a second time)
| currentYear.value = parseInt(displayedYear.value) | ||
| setCurrentYearAndDispatchToStore(currentYear.value) |
There was a problem hiding this comment.
currentYear is already set within 'setCurrentYearAndDispatchToStore', there is no need to assign its value here.
b5d2302 to
17b712b
Compare
ltshb
left a comment
There was a problem hiding this comment.
Code is good just about the language changes in the tippy. Also the translation needs to be updated, you can send me the translation by slack and I add them to the spreadsheet.
| tippyInstance = tippy(yearCursorInput.value, { | ||
| content: | ||
| i18n.t('outside_valid_year_range') + | ||
| '{' + | ||
| ALL_YEARS[0] + | ||
| '-' + | ||
| ALL_YEARS[ALL_YEARS.length - 1] + | ||
| '}', | ||
| arrow: true, | ||
| hideOnClick: false, | ||
| placement: 'bottom', | ||
| trigger: 'manual', | ||
| theme: 'danger', |
There was a problem hiding this comment.
You need to manage the content also when the language changes
53c6c99 to
931d920
Compare
ltshb
left a comment
There was a problem hiding this comment.
Just a slight tooltip formating improvment
| i18n.t('outside_valid_year_range') + | ||
| '{' + | ||
| ALL_YEARS[0] + | ||
| '-' + | ||
| ALL_YEARS[ALL_YEARS.length - 1] + | ||
| '}' |
There was a problem hiding this comment.
Perso I would not use the {} simply
return `${i18n.t('outside_valid_year_range')} ${ALL_YEARS[0]}-${ALL_YEARS[ALL_YEARS.length - 1]}`The output will look nicer less coding.
931d920 to
edcdf98
Compare
edcdf98 to
eb1587d
Compare
Test link