-
Notifications
You must be signed in to change notification settings - Fork 14
Observation form UX improvements #1079
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Observation form UX improvements #1079
Conversation
|
@Rory-Sullivan that looks pretty dang good! I'm excited to play with it in Preview. I might trim a few things but excited to test first! |
| value: Date; | ||
| } | ||
|
|
||
| interface HelpText { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this to AvalancheObservationForm and add an export to it. With the designs I've seen for the help text, it looks like we might be adding it to a lot of fields so that seems to be a place that makes the most sense to add it to for the best visibility and reusability. I've they get added to ObservationForm that might be an even better place for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, if you end up breaking out the title code into a new component, this should go in that component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the future we may end help text to not just obs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a new component called FieldLabel this HelpText interface is now along side that new component. I think that resolves this comment?
| return reverseLookup(AvalancheSize, value); | ||
| }; | ||
|
|
||
| const avalancheSizeHelpTextHtml = String.raw` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There a filed called helpStrings.ts that contains the help strings for the tooltip used in other areas. I would add a section there called avalancheObservations and add all the help strings being used in this file under that section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| ]} | ||
| maximumDate={today} | ||
| required | ||
| helpText={{title: 'Occurrence date', contentHtml: 'To the best of your knowledge, when did the avalanche occur?'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add to helpStrings.ts (see comment below) and wrap in <p></p> tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| items={Object.values(AvalancheAspect).map(aspect => ({label: FormatAvalancheAspect(aspect), value: aspect}))} | ||
| quickPickItems={Object.values(AvalancheAspect).map(aspect => ({label: FormatAvalancheAspect(aspect), value: aspect}))} | ||
| required | ||
| helpText={{title: 'Aspect', contentHtml: 'Provide primary or average aspect of the slope where the avalanche released.'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add to helpStrings.ts (see comment below) and wrap in <p></p> tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| borderWidth={1} | ||
| disabled={disabled}> | ||
| <BodyBlack> | ||
| <AntDesign name="calendar" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be something that's more of a personal preference but I wonder if adding "Other" along with the icon would make it even clearer that this will open the calendar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| // it from top margin of the HStack to stop the first row of buttons from being offset down. | ||
| const rowMargin = 5; | ||
|
|
||
| let isQuickPick = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change this to make it a State and call it something like isQuickPickSelected and have it updated in the quickPickSelectionHandlers. It makes it more clear what it's controlling and that it's updating the state of something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this one I decided to make isQuickPickSelected a memoized value that gets updated when the field value changes. Making it a state value and trying to update that from the selection handler is a little tricky because the user is able select one of the quick pick dates from the generic date selector. Happy to discuss this more though if you have particular thoughts on it.
| <HStack> | ||
| <BodySmBlack> | ||
| {label ?? name} | ||
| {required && ' *'} | ||
| </BodySmBlack> | ||
| {helpText && <InfoTooltip title={helpText.title} content={helpText.contentHtml} size={14} htmlStyle={{textAlign: 'left'}} />} | ||
| </HStack> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This HStack could be broken out into a FieldTitle component so that it isn't repeated across the components. It also could be where the HelpText interface is declared
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I decided to call the new component FieldLabel.
| if (Array.isArray(field.value)) { | ||
| throw new Error('ButtonSelectField cannot support multiselects at this time'); | ||
| } | ||
| if (otherItems) { | ||
| quickPickItems.forEach(item => { | ||
| if (otherItems.find(x => x.value === item.value)) { | ||
| throw new Error('ButtonSelectField quickPickItems and otherItems must be distinct lists'); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not a big deal, but should these be in a useEffect so it's not checked on every render?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code has moved to the SelectField component, but it has been wrapped in a useEffect hook.
| {label: 'Natural', value: 'N'}, | ||
| {label: 'Unknown', value: 'U'}, | ||
| {label: 'Skier', value: 'AS'}, | ||
| {label: 'Snowboarder', value: 'AR'}, | ||
| {label: 'Snowmobile', value: 'AM'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of hard coding these we should slice and map the values like we're doing below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
It doesn't have to be a part of this PR, but if these changes are added to the |
| useEffect(() => { | ||
| if (required && quickPickDates.length > 0) { | ||
| setValue(name, quickPickDates[0].value, {shouldValidate: true, shouldDirty: true, shouldTouch: true}); | ||
| } | ||
| }, [name, required, quickPickDates, setValue]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There needs to be a check in here to see if field.value is already set to something. If a user saves an avalanche obs and then taps on the edit icon to bring the form up again, this will override their previous selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| useEffect(() => { | ||
| if (required && quickPickItems.length > 0) { | ||
| setValue(name, quickPickItems[0].value, {shouldValidate: true, shouldDirty: true, shouldTouch: true}); | ||
| } | ||
| }, [name, required, quickPickItems, setValue]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here about checking if a value is set already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| placeholder: 'Describe the location of the avalanche.', | ||
| multiline: true, | ||
| }} | ||
| required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that there's a preexisting bug here that allows users to save the avalanche without setting the location field. I'll open an issue on myself to fix this once this code gets checked in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the behavior of this one is a little weird and I am not entirely sure what is going on.
- Add ButtonSelectField component for quick selects - Convert aspect and avalanche size fields to button select
- Add ButtonSelectDateField component for quick selects of dates - Convert date field to button select
Updates the format function used to display the date of an avalanche. The avalanche date field is sent from the backend without time or timezone information, so when it is converted to UTC then back to a local date it is off by one day. Renames a function that is used to convert observation start dates to a local date string to make it more generic.
…rm (NWACus#961) - Update button select component to have an optional select dropdown for cases with many items. - Update trigger field on avalanche submission form to have quick select items and dropdown select.
96515c0 to
d3674ed
Compare
- Add 'other' text to general date picker - Fix bug that clears date selection when there is a preset value - Memoize is quick pick selected variable
- Create new FieldLabel component - Use new component in various Field components
Move help text to helpStrings.ts file.
Update trigger quick pick items to map of schema items instead of hard coded list.
I have merged the |
Remove '2 days ago' from quick pick date items.
|
Latest change also removes '2 days ago' from the quick pick date items per a request on Slack. |

This PR is currently focused on the avalanche observation form, though I expect we may want to roll some changes into the observation form as well. It addresses the following issues;
InfoTooltipcomponent that is is used for things like the help text on danger ratings in the observation detail view. @rustynwac you mentioned doing some design work on this item, I'm super happy to adjust this to whatever comes out of that, I just took the path of least resistance to get this functionality in there.From a technical perspective this PR adds two new components
ButtonSelectFieldandButtonSelectDateField.ButtonSelectFieldis very similar to theSelectFieldcomponent but it adds the ability to have quick select items as well as the drop down select. The main limitation to theButtonSelectFieldcomponent overSelectFieldis that it does not support multi-select fields.ButtonSelectDateFieldadds quick pick items to a date selector.It also adjusts existing field components to improve the visibility of required fields and errors.