[A11y] Add Save button to Report settings single-select pages (Write capability, Notification preference, Visibility) #94557
+75
−52
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,7 +23,7 @@ import CONST from '@src/CONST'; | |||||||
| import {DYNAMIC_ROUTES} from '@src/ROUTES'; | ||||||||
| import type {RoomVisibility} from '@src/types/onyx/Report'; | ||||||||
|
|
||||||||
| import React, {useMemo, useState} from 'react'; | ||||||||
|
|
||||||||
| type DynamicVisibilityProps = WithReportOrNotFoundProps; | ||||||||
|
|
||||||||
|
|
@@ -35,6 +35,9 @@ function DynamicVisibilityPage({report}: DynamicVisibilityProps) { | |||||||
|
|
||||||||
| const {showConfirmModal} = useConfirmModal(); | ||||||||
|
|
||||||||
| const [draftVisibility, setDraftVisibility] = useState<RoomVisibility | undefined>(undefined); | ||||||||
| const selectedVisibility = draftVisibility ?? report?.visibility; | ||||||||
|
|
||||||||
| const visibilityOptions = useMemo( | ||||||||
| () => | ||||||||
| Object.values(CONST.REPORT.VISIBILITY) | ||||||||
|
|
@@ -44,36 +47,43 @@ function DynamicVisibilityPage({report}: DynamicVisibilityProps) { | |||||||
| value: visibilityOption, | ||||||||
| alternateText: translate(`newRoomPage.${visibilityOption}Description`), | ||||||||
| keyForList: visibilityOption, | ||||||||
| isSelected: visibilityOption === selectedVisibility, | ||||||||
| })), | ||||||||
| [translate, selectedVisibility], | ||||||||
| ); | ||||||||
|
|
||||||||
| const goBack = () => { | ||||||||
| Navigation.goBack(backPath); | ||||||||
| }; | ||||||||
|
|
||||||||
| const saveVisibility = async () => { | ||||||||
|
Contributor
There was a problem hiding this comment. Public confirmation modal triggers when nothing changed If a room is already public and the user taps Save without selecting a different option,
Contributor
Author
There was a problem hiding this comment. Fixed by the same change in d8abb61. Save is now disabled while |
||||||||
| if (!report || !selectedVisibility) { | ||||||||
| return; | ||||||||
| } | ||||||||
|
|
||||||||
| // Selecting Public is a sensitive change, so it still has to be confirmed before we persist it. | ||||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
Contributor
Author
There was a problem hiding this comment. Done in 0f72407 — added the blank line before the comment. |
||||||||
| if (selectedVisibility === CONST.REPORT.VISIBILITY.PUBLIC) { | ||||||||
| const result = await showConfirmModal({ | ||||||||
| title: translate('common.areYouSure'), | ||||||||
| prompt: translate('newRoomPage.publicDescription'), | ||||||||
| confirmText: translate('common.yes'), | ||||||||
| cancelText: translate('common.no'), | ||||||||
| shouldShowCancelButton: true, | ||||||||
| danger: true, | ||||||||
| }); | ||||||||
| if (result.action !== ModalActions.CONFIRM) { | ||||||||
| return; | ||||||||
| } | ||||||||
| } | ||||||||
| updateRoomVisibility(report.reportID, report.visibility, selectedVisibility); | ||||||||
| setNavigationActionToMicrotaskQueue(goBack); | ||||||||
| }; | ||||||||
|
|
||||||||
| const confirmButtonOptions = { | ||||||||
| showButton: true, | ||||||||
| text: translate('common.save'), | ||||||||
| onConfirm: saveVisibility, | ||||||||
| isDisabled: selectedVisibility === report?.visibility, | ||||||||
| }; | ||||||||
|
|
||||||||
| return ( | ||||||||
|
|
@@ -89,16 +99,12 @@ function DynamicVisibilityPage({report}: DynamicVisibilityProps) { | |||||||
| <SelectionList | ||||||||
| shouldPreventDefaultFocusOnSelectRow | ||||||||
| data={visibilityOptions} | ||||||||
| onSelectRow={(option) => setDraftVisibility(option.value)} | ||||||||
| confirmButtonOptions={confirmButtonOptions} | ||||||||
| shouldSingleExecuteRowSelect | ||||||||
| initiallyFocusedItemKey={report?.visibility} | ||||||||
| ListItem={SingleSelectListItem} | ||||||||
| addBottomSafeAreaPadding | ||||||||
| /> | ||||||||
| </FullPageNotFoundView> | ||||||||
| </ScreenWrapper> | ||||||||
|
|
||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,37 +17,43 @@ import type {WithReportOrNotFoundProps} from '@pages/inbox/report/withReportOrNo | |
|
|
||
| import CONST from '@src/CONST'; | ||
| import {DYNAMIC_ROUTES} from '@src/ROUTES'; | ||
| import type {WriteCapability} from '@src/types/onyx/Report'; | ||
|
|
||
| import React, {useState} from 'react'; | ||
|
|
||
| type DynamicWriteCapabilityPageProps = WithReportOrNotFoundProps; | ||
|
|
||
| function DynamicWriteCapabilityPage({report, policy}: DynamicWriteCapabilityPageProps) { | ||
| const backPath = useDynamicBackPath(DYNAMIC_ROUTES.REPORT_SETTINGS_WRITE_CAPABILITY.path); | ||
| const {translate} = useLocalize(); | ||
| const currentWriteCapability = report?.writeCapability ?? CONST.REPORT.WRITE_CAPABILITIES.ALL; | ||
|
|
||
| const [draftWriteCapability, setDraftWriteCapability] = useState<WriteCapability | undefined>(undefined); | ||
| const selectedWriteCapability = draftWriteCapability ?? currentWriteCapability; | ||
| const writeCapabilityOptions = Object.values(CONST.REPORT.WRITE_CAPABILITIES).map((value) => ({ | ||
| value, | ||
| text: translate(`writeCapabilityPage.writeCapability.${value}`), | ||
| keyForList: value, | ||
| isSelected: value === selectedWriteCapability, | ||
| })); | ||
| const isReportArchived = useReportIsArchived(report.reportID); | ||
| const isAbleToEdit = canEditWriteCapability(report, policy, isReportArchived); | ||
|
|
||
| const goBack = () => { | ||
| Navigation.goBack(backPath); | ||
| }; | ||
|
|
||
| const saveWriteCapability = () => { | ||
| updateWriteCapabilityUtil(report, selectedWriteCapability); | ||
| goBack(); | ||
| }; | ||
|
|
||
| const confirmButtonOptions = { | ||
|
Contributor
There was a problem hiding this comment. Save enabled with no changes All three pages let the user tap Save without selecting a different option, which fires the update API with old === new. Would it make sense to disable the button while
Contributor
Author
There was a problem hiding this comment. Good call — done in d8abb61. All three Save buttons are now disabled whenever the selected value equals the current live value, i.e.
|
||
| showButton: true, | ||
| text: translate('common.save'), | ||
| onConfirm: saveWriteCapability, | ||
| isDisabled: selectedWriteCapability === currentWriteCapability, | ||
| }; | ||
|
|
||
| return ( | ||
| <ScreenWrapper | ||
|
|
@@ -63,9 +69,11 @@ function DynamicWriteCapabilityPage({report, policy}: DynamicWriteCapabilityPage | |
| <SelectionList | ||
| data={writeCapabilityOptions} | ||
| ListItem={SingleSelectListItem} | ||
| onSelectRow={(option) => setDraftWriteCapability(option.value)} | ||
| confirmButtonOptions={confirmButtonOptions} | ||
| shouldSingleExecuteRowSelect | ||
| initiallyFocusedItemKey={currentWriteCapability} | ||
| addBottomSafeAreaPadding | ||
| /> | ||
| </FullPageNotFoundView> | ||
| </ScreenWrapper> | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
On devices with a bottom gesture/home-indicator inset, this new fixed Save footer receives only its regular
pb5spacing because the surroundingScreenWrapperexplicitly disables bottom safe-area padding and theSelectionListdoes not setaddBottomSafeAreaPadding. The same combination occurs in all three changed pages, so their Save buttons can intrude into the unsafe area; passaddBottomSafeAreaPaddingto each list or let the wrapper provide the inset.Useful? React with 👍 / 👎.
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.
@MelvinBot it is a valid concern. Please resolve 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.
Agreed — fixed in 8fe8710. Each of the three pages sets
includeSafeAreaPaddingBottom={false}on itsScreenWrapper, so theSelectionList's fixed Save footer (FixedFooter) was getting only its basepb5spacing and no bottom inset. PassingaddBottomSafeAreaPaddingto each list makesFixedFooteradd the device bottom inset (viauseBottomSafeSafeAreaPaddingStyle→ rawuseSafeAreaPaddings), so the Save button no longer intrudes into the home-indicator/gesture area. Because the wrapper's bottom padding is disabled, there's no double-count, and since a footer is present the list's own container padding is skipped — the inset lands exactly on the footer. This matches the established pattern (e.g.CardTypeStep).DynamicNotificationPreferencePage.tsx:84DynamicWriteCapabilityPage.tsx:77DynamicVisibilityPage.tsx:108