-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: action sheet bottom blank space #7390
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
Open
OtavioStasiak
wants to merge
29
commits into
develop
Choose a base branch
from
fix/actionsheet-safe-area-spacing
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 18 commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
3df8fb8
feat: install react-native-owl
OtavioStasiak 855d8c1
fix: blank space on actionSheet
OtavioStasiak f55ea6c
feat: visual regression test start
OtavioStasiak 28c992d
feat: visual regression run/compare + e2e-style CI flow
OtavioStasiak 8f8d4ff
Merge branch 'develop' into fix/actionsheet-safe-area-spacing
OtavioStasiak c2deb72
ci: gate visual regression behind manual approval hold
OtavioStasiak ebab06a
ci: fix visual regression android build + ios simulator boot
OtavioStasiak f66caab
ci: emit clean baseline-only artifact for owl update runs
OtavioStasiak 3298251
ci: harden owl renders for deterministic baselines
OtavioStasiak 039c25f
ci: mask OS status bar in owl diffs so baselines verify the app
OtavioStasiak b54b6f7
Merge branch 'develop' into fix/actionsheet-safe-area-spacing
OtavioStasiak f5d4590
fix: blank space on actionSheet
OtavioStasiak 6e99aa2
test: guard action-sheet bottom safe-area spacing
OtavioStasiak c1399c5
test: also guard paddingBottom in action-sheet spacing tests
OtavioStasiak 6544e0b
Merge branch 'develop' into fix/actionsheet-safe-area-spacing
OtavioStasiak d7adf67
fix: ServersList Add Server button clipped in landscape
OtavioStasiak cbd23fa
test: lock UserNotificationPreferences sheet paddingBottom contract
OtavioStasiak e6c40e2
fix: listPicker
OtavioStasiak b63cdf2
fix: tests
OtavioStasiak bebcbc7
fix: android specific padding issues
OtavioStasiak 1c4c531
fix: action sheet last row clipped behind Android nav bar
OtavioStasiak df87d03
feat: improve e2e tests
OtavioStasiak 1559d51
Merge branch 'develop' into fix/actionsheet-safe-area-spacing
OtavioStasiak e77c60f
refactor: dedupe action sheet max-height fraction in servers list
OtavioStasiak 153eac9
refactor: simplify getActionSheetBottomInset to take bottom inset dir…
OtavioStasiak 8481335
fix: use nullish coalescing for action sheet content padding
OtavioStasiak c879f00
refactor: drop initialWindowMetrics workaround from action sheet bott…
OtavioStasiak a96ce16
Merge branch 'develop' into fix/actionsheet-safe-area-spacing
OtavioStasiak 6e5ca3f
Merge branch 'develop' into fix/actionsheet-safe-area-spacing
OtavioStasiak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| appId: ${APP_ID} | ||
| name: Action Sheet Safe Area | ||
| onFlowStart: | ||
| - runFlow: '../../helpers/setup.yaml' | ||
| onFlowComplete: | ||
| - evalScript: ${output.utils.deleteCreatedUsers()} | ||
| tags: | ||
| - test-7 | ||
|
|
||
| # Regression for the action-sheet bottom safe-area spacing fix. The sheet | ||
| # (TrueSheet / BottomSheetContent) owns the bottom safe-area inset; content must | ||
| # not add its own bottom margin, which would either leave a blank band or push | ||
| # the bottom-most row off-screen. Maestro can't measure a pixel gap, so the guard | ||
| # is that each sheet opens and its bottom-most content stays fully visible. | ||
|
|
||
| --- | ||
| - evalScript: ${output.user = output.utils.createUser()} | ||
|
|
||
| - runFlow: | ||
| file: '../../helpers/login-with-deeplink.yaml' | ||
| env: | ||
| USERNAME: ${output.user.username} | ||
| PASSWORD: ${output.user.password} | ||
|
|
||
| - extendedWaitUntil: | ||
| visible: | ||
| id: 'rooms-list-view' | ||
| timeout: 60000 | ||
|
|
||
| # Workspaces (servers list) action sheet — the bottom "Add server" row must be visible. | ||
| - extendedWaitUntil: | ||
| visible: | ||
| id: 'rooms-list-header-servers-list-button' | ||
| timeout: 60000 | ||
| - tapOn: | ||
| id: 'rooms-list-header-servers-list-button' | ||
| - waitForAnimationToEnd: | ||
| timeout: 10000 | ||
| - extendedWaitUntil: | ||
| visible: | ||
| id: 'action-sheet' | ||
| timeout: 60000 | ||
| - extendedWaitUntil: | ||
| visible: | ||
| id: 'rooms-list-header-servers-list' | ||
| timeout: 60000 | ||
| - assertVisible: | ||
| id: 'rooms-list-header-server-add' | ||
|
|
||
| # Dismiss the sheet before moving on. | ||
| - swipe: | ||
| direction: DOWN | ||
| duration: 500 | ||
| - waitForAnimationToEnd: | ||
| timeout: 10000 | ||
|
|
||
| # Directory filter action sheet — the bottom radio row must be visible. | ||
| - extendedWaitUntil: | ||
| visible: | ||
| id: 'rooms-list-view-directory' | ||
| timeout: 60000 | ||
| - tapOn: | ||
| id: 'rooms-list-view-directory' | ||
| - extendedWaitUntil: | ||
| visible: | ||
| id: 'directory-view' | ||
| timeout: 60000 | ||
| - extendedWaitUntil: | ||
| visible: | ||
| id: 'directory-view-filter' | ||
| timeout: 60000 | ||
| - tapOn: | ||
| id: 'directory-view-filter' | ||
| - waitForAnimationToEnd: | ||
| timeout: 10000 | ||
| - extendedWaitUntil: | ||
| visible: | ||
| id: 'action-sheet' | ||
| timeout: 60000 | ||
| - assertVisible: | ||
| id: 'directory-switch-channels' | ||
| - assertVisible: | ||
| id: 'directory-switch-teams' |
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 |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import { render, screen } from '@testing-library/react-native'; | ||
| import { ScrollView, StyleSheet } from 'react-native'; | ||
|
|
||
| import DirectoryOptions from './Options'; | ||
|
|
||
| // The directory filter is shown as action-sheet content. The sheet already applies the | ||
| // bottom safe-area inset, so the List.Container must not add `marginBottom: insets.bottom` | ||
| // (double-counting leaves a blank band — the bug this guards against). Inset mocked to a | ||
| // non-zero value so a regression re-adding the margin would surface as marginBottom: 34. | ||
| jest.mock('react-native-safe-area-context', () => ({ | ||
| useSafeAreaInsets: () => ({ top: 0, bottom: 34, left: 0, right: 0 }) | ||
| })); | ||
|
|
||
| describe('DirectoryView Options — action sheet bottom spacing', () => { | ||
| it('does not bake the bottom safe-area inset into the list container', () => { | ||
| render( | ||
| <DirectoryOptions | ||
| type='channels' | ||
| globalUsers={false} | ||
| isFederationEnabled={false} | ||
| changeType={jest.fn()} | ||
| toggleWorkspace={jest.fn()} | ||
| /> | ||
| ); | ||
|
|
||
| const container = screen.UNSAFE_getByType(ScrollView); | ||
| const style = StyleSheet.flatten(container.props.contentContainerStyle); | ||
| // Neither margin nor padding: this view drops the inset entirely, so a | ||
| // margin->padding swap would still inflate the bottom spacing and must fail. | ||
| expect(style.marginBottom).toBeUndefined(); | ||
| expect(style.paddingBottom).toBeUndefined(); | ||
| }); | ||
| }); | ||
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 |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import { fireEvent, render, screen } from '@testing-library/react-native'; | ||
| import { StyleSheet } from 'react-native'; | ||
|
|
||
| import ListPicker from './ListPicker'; | ||
|
|
||
| // Non-zero bottom inset: the action sheet (TrueSheet / BottomSheetContent) already | ||
| // applies the bottom safe-area inset to its content, so the picker content must NOT | ||
| // add its own `marginBottom: insets.bottom` — doing so double-counts and leaves a | ||
| // blank band at the bottom of the sheet (the bug this guards against). With the inset | ||
| // mocked to 34, a regression that re-adds the margin would surface as marginBottom: 34. | ||
| jest.mock('react-native-safe-area-context', () => ({ | ||
| useSafeAreaInsets: () => ({ top: 0, bottom: 34, left: 0, right: 0 }) | ||
| })); | ||
|
|
||
| jest.mock('../../containers/ActionSheet', () => ({ useActionSheet: jest.fn() })); | ||
| const mockUseActionSheet = require('../../containers/ActionSheet').useActionSheet as jest.Mock; | ||
|
|
||
| describe('MediaAutoDownloadView ListPicker — action sheet bottom spacing', () => { | ||
| const showActionSheet = jest.fn(); | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| mockUseActionSheet.mockReturnValue({ showActionSheet, hideActionSheet: jest.fn() }); | ||
| }); | ||
|
|
||
| it('does not bake the bottom safe-area inset into the sheet content', () => { | ||
| render(<ListPicker value='wifi' title='Images' testID='media-picker' onChangeValue={jest.fn()} />); | ||
|
|
||
| fireEvent.press(screen.getByTestId('media-picker')); | ||
|
|
||
| expect(showActionSheet).toHaveBeenCalledTimes(1); | ||
| const { children } = showActionSheet.mock.calls[0][0]; | ||
| const style = StyleSheet.flatten(children.props.style); | ||
| // Neither margin nor padding: this view drops the inset entirely, so a | ||
| // margin->padding swap would still inflate the bottom spacing and must fail. | ||
| expect(style.marginBottom).toBeUndefined(); | ||
| expect(style.paddingBottom).toBeUndefined(); | ||
| }); | ||
| }); |
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
44 changes: 44 additions & 0 deletions
44
app/views/RoomsListView/components/serversListLayout.test.ts
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 |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import { getServersListMaxHeight, SERVERS_LIST_MAX_ROWS, SERVERS_LIST_ROW_HEIGHT } from './serversListLayout'; | ||
|
|
||
| const FULL_HEIGHT = SERVERS_LIST_MAX_ROWS * SERVERS_LIST_ROW_HEIGHT; | ||
|
|
||
| // Reserve for the Workspaces header + separator + Add Server button + sheet handle. | ||
| // Must match SERVERS_LIST_CHROME_HEIGHT in serversListLayout.ts. | ||
| const CHROME = 150; | ||
| const SHEET_MAX_FRACTION = 0.75; | ||
|
|
||
| describe('getServersListMaxHeight', () => { | ||
| it('keeps the full MAX_ROWS height in portrait (plenty of room)', () => { | ||
| // iPhone-ish portrait height | ||
| expect(getServersListMaxHeight(844)).toBe(FULL_HEIGHT); | ||
| // Tall window is still capped at the row max, not the window | ||
| expect(getServersListMaxHeight(2000)).toBe(FULL_HEIGHT); | ||
| }); | ||
|
|
||
| it('shrinks the list in a short (landscape) window so the Add Server button still fits', () => { | ||
| const landscapeHeight = 390; // phone landscape | ||
| const maxHeight = getServersListMaxHeight(landscapeHeight); | ||
|
|
||
| // Constrained below the full height... | ||
| expect(maxHeight).toBeLessThan(FULL_HEIGHT); | ||
| // ...so that list + chrome fits within the sheet's max height (button not clipped). | ||
| expect(maxHeight + CHROME).toBeLessThanOrEqual(landscapeHeight * SHEET_MAX_FRACTION); | ||
| }); | ||
|
|
||
| it('regression: 3 workspaces in landscape leave the Add Server button visible', () => { | ||
| const landscapeHeight = 390; | ||
| const threeRows = 3 * SERVERS_LIST_ROW_HEIGHT; // 204, what the old fixed 306 cap would have shown | ||
| const maxHeight = getServersListMaxHeight(landscapeHeight); | ||
|
|
||
| // The list is capped shorter than 3 full rows, so it scrolls instead of pushing | ||
| // the button off-screen — the bug being fixed. | ||
| expect(maxHeight).toBeLessThan(threeRows); | ||
| expect(maxHeight + CHROME).toBeLessThanOrEqual(landscapeHeight * SHEET_MAX_FRACTION); | ||
| }); | ||
|
|
||
| it('never collapses below a single row', () => { | ||
| // Even an extremely short window keeps at least one row visible. | ||
| expect(getServersListMaxHeight(100)).toBe(SERVERS_LIST_ROW_HEIGHT); | ||
| expect(getServersListMaxHeight(0)).toBe(SERVERS_LIST_ROW_HEIGHT); | ||
| }); | ||
| }); |
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 |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| // Layout math for the Workspaces (servers list) action sheet, kept dependency-free | ||
|
OtavioStasiak marked this conversation as resolved.
|
||
| // so it can be unit-tested without the redux/database tree the component needs. | ||
|
OtavioStasiak marked this conversation as resolved.
|
||
|
|
||
| export const SERVERS_LIST_ROW_HEIGHT = 68; | ||
| export const SERVERS_LIST_MAX_ROWS = 4.5; | ||
|
|
||
| // Vertical space the sheet needs for everything that is NOT the scrollable server | ||
| // list: the "Workspaces" header, the separator, the Add Server button block, and | ||
| // the sheet handle. Reserved so the button is never pushed off-screen. | ||
|
OtavioStasiak marked this conversation as resolved.
Outdated
|
||
| const SERVERS_LIST_CHROME_HEIGHT = 150; | ||
|
OtavioStasiak marked this conversation as resolved.
Outdated
|
||
|
|
||
| // Mirrors ACTION_SHEET_MAX_HEIGHT_FRACTION in useActionSheetDetents: a children | ||
| // action sheet can grow to at most this fraction of the window height. | ||
| const ACTION_SHEET_MAX_HEIGHT_FRACTION = 0.75; | ||
|
OtavioStasiak marked this conversation as resolved.
Outdated
|
||
|
|
||
| /** | ||
| * Max height for the servers FlatList. In portrait there is plenty of room, so it | ||
| * stays at the full MAX_ROWS cap (unchanged behaviour). In a short window | ||
| * (landscape, split-view) it shrinks so the header + Add Server button still fit | ||
| * inside the sheet's max height and the list scrolls instead of clipping the | ||
| * button — e.g. landscape with 3+ workspaces connected. Never smaller than one row. | ||
| */ | ||
| export const getServersListMaxHeight = (windowHeight: number): number => { | ||
|
OtavioStasiak marked this conversation as resolved.
|
||
| const fullHeight = SERVERS_LIST_MAX_ROWS * SERVERS_LIST_ROW_HEIGHT; | ||
| const availableForList = windowHeight * ACTION_SHEET_MAX_HEIGHT_FRACTION - SERVERS_LIST_CHROME_HEIGHT; | ||
| return Math.max(SERVERS_LIST_ROW_HEIGHT, Math.min(fullHeight, availableForList)); | ||
| }; | ||
40 changes: 40 additions & 0 deletions
40
app/views/UserNotificationPreferencesView/ListPicker.test.tsx
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 |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import { fireEvent, render, screen } from '@testing-library/react-native'; | ||
| import { StyleSheet } from 'react-native'; | ||
|
|
||
| import ListPicker from './ListPicker'; | ||
|
|
||
| // The sheet already applies the bottom safe-area inset to its content, so the picker | ||
| // content must not add a `marginBottom: insets.bottom` (that double-counts and leaves | ||
| // a blank band — the bug this guards against). This view intentionally keeps a | ||
| // `paddingBottom` for now, so the guard is specifically on marginBottom. Inset mocked | ||
| // to 34 so a regression re-adding the margin would surface as marginBottom: 34. | ||
| jest.mock('react-native-safe-area-context', () => ({ | ||
| useSafeAreaInsets: () => ({ top: 0, bottom: 34, left: 0, right: 0 }) | ||
| })); | ||
|
|
||
| jest.mock('../../containers/ActionSheet', () => ({ useActionSheet: jest.fn() })); | ||
| const mockUseActionSheet = require('../../containers/ActionSheet').useActionSheet as jest.Mock; | ||
|
|
||
| describe('UserNotificationPreferencesView ListPicker — action sheet bottom spacing', () => { | ||
| const showActionSheet = jest.fn(); | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| mockUseActionSheet.mockReturnValue({ showActionSheet, hideActionSheet: jest.fn() }); | ||
| }); | ||
|
|
||
| it('does not bake the bottom safe-area inset as a margin into the sheet content', () => { | ||
| render( | ||
| <ListPicker preference='desktopNotifications' value='all' title='Alert' testID='notif-picker' onChangeValue={jest.fn()} /> | ||
| ); | ||
|
|
||
| fireEvent.press(screen.getByTestId('notif-picker')); | ||
|
|
||
| expect(showActionSheet).toHaveBeenCalledTimes(1); | ||
| const { children } = showActionSheet.mock.calls[0][0]; | ||
| const style = StyleSheet.flatten(children.props.style); | ||
| expect(style.marginBottom).toBeUndefined(); | ||
| // This view intentionally keeps the bottom safe-area inset as padding, so lock that contract. | ||
| expect(style.paddingBottom).toBe(34); | ||
| }); | ||
| }); |
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
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.
Uh oh!
There was an error while loading. Please reload this page.