Skip to content

fix: required field error is not displayed when clearing the field #61653

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 15, 2025
Merged

fix: required field error is not displayed when clearing the field #61653

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6a63fd8
fix: required field error is not displayed when clearing the field
daledah May 8, 2025
9bf5d7c
fix: hide clear buttons
daledah May 8, 2025
213fdeb
fix: lint
daledah May 8, 2025
8d7b746
Merge branch 'main' into fix/60243
daledah May 14, 2025
3f5d7af
fix: canBeMissing
daledah May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/components/Form/FormProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,9 +344,7 @@ function FormProvider(
defaultValue: inputProps.uncontrolled ? inputProps.defaultValue : undefined,
onTouched: (event) => {
if (!inputProps.shouldSetTouchedOnBlurOnly) {
setTimeout(() => {
setTouchedInput(inputID);
}, VALIDATE_DELAY);
setTouchedInput(inputID);
}
inputProps.onTouched?.(event);
},
Expand Down
3 changes: 1 addition & 2 deletions src/pages/GroupChatNameEditPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ function GroupChatNameEditPage({report}: GroupChatNameEditPageProps) {
// In this case its better to use empty string as the reportID if there is no reportID
const reportID = report?.reportID;
const isUpdatingExistingReport = !!reportID;
const [groupChatDraft] = useOnyx(ONYXKEYS.NEW_GROUP_CHAT_DRAFT, {initialValue: getGroupChatDraft()});
const [groupChatDraft] = useOnyx(ONYXKEYS.NEW_GROUP_CHAT_DRAFT, {initialValue: getGroupChatDraft(), canBeMissing: false});

const styles = useThemeStyles();
const {translate} = useLocalize();
Expand Down Expand Up @@ -101,7 +101,6 @@ function GroupChatNameEditPage({report}: GroupChatNameEditPageProps) {
inputID={INPUT_IDS.NEW_CHAT_NAME}
role={CONST.ROLE.PRESENTATION}
ref={inputCallbackRef}
shouldShowClearButton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can keep the clear button here because an empty string is a valid option here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @trjExpensify

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to keep the clear X icon here. We said only:

This is neither, right? CC: @Expensify/design for vis

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we can probably get rid of it here based on our little framework. Curious what the other designers think though!

I could see it being helpful here in the case where we think it's likely that you would rename a group to something custom. But I am also fine following our framework for consistency's sake.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want the group name to take its default name you'd have to clear the input and submit an empty string. I think the clear button serves as an indication here that is an empty string is allowed.

Generally I would say that if an empty string is meaningful we should have a clear button.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want the group name to take its default name you'd have to clear the input and submit an empty string.

Personally I think this is pretty weird behavior and doesn't feel expected to me. It would make more sense to me if you could not save an empty string here.

/>
</FormProvider>
</ScreenWrapper>
Expand Down
1 change: 0 additions & 1 deletion src/pages/Search/SavedSearchRenamePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ function SavedSearchRenamePage({route}: {route: {params: {q: string; name: strin
onChangeText={(renamedName) => setNewName(renamedName)}
ref={inputCallbackRef}
defaultValue={name}
shouldShowClearButton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still need this as it's coming from #50163

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @trjExpensify

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dannymcclain as the linked issue was yours. Are we deeming this one a "search input field" because it technically has the search query named stored before you rename it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I don't think so. IMO this input is more like a Group name than the search input, so we can probably ditch this one too. cc @Expensify/design

(I think in our little framework we decided that we're not super worried about the usability of clearing a long text field, so we decided to remove the clear button from inputs like this, group names, etc)

/>
</FormProvider>
</ScreenWrapper>
Expand Down
1 change: 0 additions & 1 deletion src/pages/TripChatNameEditPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ function TripChatNameEditPage({report}: TripChatNameEditPageProps) {
inputID={INPUT_IDS.NEW_CHAT_NAME}
role={CONST.ROLE.PRESENTATION}
ref={inputCallbackRef}
shouldShowClearButton
/>
</FormProvider>
</ScreenWrapper>
Expand Down
Loading