Skip to content

Clean up navigation logic (4/x): Tidy up our use of ZulipStatusBar a bit more. #4468

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
chrisbobbe merged 17 commits into from
Apr 16, 2021
Merged

Clean up navigation logic (4/x): Tidy up our use of ZulipStatusBar a bit more. #4468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e918500
Screen [nfc]: Convert to use Hooks (1/2).
chrisbobbe Mar 1, 2021
565c125
Screen [nfc]: Convert to use Hooks (2/2).
chrisbobbe Feb 4, 2021
69a30e7
Screen: Use `SafeAreaView`.
chrisbobbe Feb 4, 2021
05bd898
MainTabsScreen: Use `SafeAreaView`.
chrisbobbe Apr 9, 2021
a40bc52
ZulipStatusBar [nfc]: Expand/improve jsdoc.
chrisbobbe Mar 2, 2021
08f0feb
ChatNavBar [nfc]: Put `ZulipStatusBar` in `ChatNavBar`.
chrisbobbe Feb 2, 2021
ebda227
Screen [nfc]: Set status bar color in `Modal(Search)NavBar` instead.
chrisbobbe Feb 3, 2021
a86b395
Lightbox: Move `ZulipStatusBar` callsite here, from `LightboxScreen`.
chrisbobbe Feb 2, 2021
91920e2
Lightbox: Show and hide the status bar with the header/footer.
chrisbobbe Feb 3, 2021
d24f96c
types [nfc]: Add jsdoc for `EditMessage`.
chrisbobbe Feb 3, 2021
771f3c2
ChatScreen: Store `editMessage` as a nav param, not React state.
chrisbobbe Feb 3, 2021
bbab920
KeyboardAvoider [nfc]: Write a jsdoc.
chrisbobbe Mar 2, 2021
70eca07
KeyboardAvoider [nfc]: Allow setting `keyboardVerticalOffset`.
chrisbobbe Feb 4, 2021
5380150
ChatScreen [nfc]: Slightly move a definition.
chrisbobbe Feb 3, 2021
188e238
ChatScreen [nfc]: Remove unnecessary wrapping `View`.
chrisbobbe Feb 3, 2021
5bd0274
ZulipStatusBar: Set default background/visibility once, in outer scope.
chrisbobbe Apr 9, 2021
c1fae32
FullScreenLoading [nfc]: Move `ZulipStatusBar` out of a `View`.
chrisbobbe Apr 9, 2021
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
8 changes: 7 additions & 1 deletion src/boot/ThemeProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type { ThemeName, Dispatch } from '../types';
import { connect } from '../react-redux';
import { getSettings } from '../directSelectors';
import { themeData, ThemeContext } from '../styles/theme';
import { ZulipStatusBar } from '../common';

type Props = $ReadOnly<{|
dispatch: Dispatch,
Expand All @@ -21,7 +22,12 @@ class ThemeProvider extends PureComponent<Props> {

render() {
const { children, theme } = this.props;
return <ThemeContext.Provider value={themeData[theme]}>{children}</ThemeContext.Provider>;
return (
<ThemeContext.Provider value={themeData[theme]}>
<ZulipStatusBar />
{children}
</ThemeContext.Provider>
);
}
}

Expand Down
78 changes: 36 additions & 42 deletions src/chat/ChatScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
/* @flow strict-local */
import React from 'react';
import { View } from 'react-native';
import { useIsFocused } from '@react-navigation/native';

import { useSelector, useDispatch } from '../react-redux';
import type { RouteProp } from '../react-navigation';
import type { AppNavigationProp } from '../nav/AppNavigator';
import styles, { ThemeContext, createStyleSheet } from '../styles';
import { ThemeContext, createStyleSheet } from '../styles';
import type { Narrow, EditMessage } from '../types';
import { KeyboardAvoider, OfflineNotice, ZulipStatusBar } from '../common';
import { KeyboardAvoider, OfflineNotice } from '../common';
import ChatNavBar from '../nav/ChatNavBar';
import MessageList from '../webview/MessageList';
import NoMessages from '../message/NoMessages';
Expand All @@ -21,11 +20,10 @@ import { canSendToNarrow } from '../utils/narrow';
import { getLoading, getSession } from '../directSelectors';
import { getFetchingForNarrow } from './fetchingSelectors';
import { getShownMessagesForNarrow, isNarrowValid as getIsNarrowValid } from './narrowsSelectors';
import { getStreamColorForNarrow } from '../subscriptions/subscriptionSelectors';

type Props = $ReadOnly<{|
navigation: AppNavigationProp<'chat'>,
route: RouteProp<'chat', {| narrow: Narrow |}>,
route: RouteProp<'chat', {| narrow: Narrow, editMessage: EditMessage | null |}>,
Copy link
Copy Markdown
Contributor Author

@chrisbobbe chrisbobbe Feb 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ChatScreen: Store `editMessage` as a nav param, not React state.

The fact that `ChatScreen`'s header changes with this bit of state
is a clue that it really wants to be a route param. When we start
using the `header` option [1] for the header, instead of smushing
the header in with the rest of the screen's content, it'll be
convenient to grab `editMessage` from the route params.

[1] https://reactnavigation.org/docs/stack-navigator/#header

We could aim to reduce the contents of editMessage, probably to just the ID of the message being edited, along the lines of what React Navigation recommends should go in these params.

I think the biggest drawback to storing the whole original content and topic equally applies in the status quo (i.e., where editMessage is stored in React state): we should be getting model data from react-redux, in a way that ensures that the UI live-updates. So, perhaps a fix can be done as a followup, or we can add a TODO. (Er, or actually it might be quite easy to do in this PR; I've just assumed it might be a bit complicated because of how tricky it was to work out what editMessage does in the first place.)

The article gives a couple of other good reasons, but some of them (like gracefully handling deep-linking) don't yet really apply to us. But I think their guidance that "You can also think of the route object like a URL" makes sense. For this screen, I'm thinking of the analogy of a URL that has something like ?msg-edit-id=12345.

Copy link
Copy Markdown
Contributor Author

@chrisbobbe chrisbobbe Feb 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In my draft that moves ModalSearchNavBar to the header, the search term is stored in the nav state. This seems fine; I'm thinking of the analogy of a URL that has something like ?q=foo.

Also, from the doc about what should go in params:

Some examples of what should be in params are:

  • [...]
  • Params for sorting, filtering data etc. when you have a list of items, e.g. navigation.navigate('Feeds', { sortBy: 'latest' })

Copy link
Copy Markdown
Contributor Author

@chrisbobbe chrisbobbe Feb 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In my draft that moves ModalSearchNavBar to the header

While at first I thought we might not want to keep the search bar in the header/nav bar, I think it's actually a fine place to put it, at least on Android.

From an Android doc:

The key functions of the app bar are as follows:

  • [...]
  • Access to important actions in a predictable way, such as search.

Copy link
Copy Markdown
Member

@gnprice gnprice Mar 2, 2021

Choose a reason for hiding this comment

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

We could aim to reduce the contents of editMessage, probably to just the ID of the message being edited

I think the thing that makes that tricky is mainly just the message content. If you look at where setEditMessage gets called (after getting renamed to startEditMessage along the way >_> -- this whole edit-message path is not a model of good software design), we have to fetch the raw Markdown content from the server at that point:

  const { raw_content } = await api.getRawMessageContent(auth, message.id);
  startEditMessage({
    id: message.id,
    content: raw_content,
    topic: message.subject,
  });

We don't have any cause to keep that information beyond the scope of one edit-message operation, so I think it is best kept as local state of one form or another. If we want the message ID in the nav state, I think the least awkward thing may be for the content and topic to go there along with it, as this PR currently does.

Copy link
Copy Markdown
Member

@gnprice gnprice Mar 2, 2021

Choose a reason for hiding this comment

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

While at first I thought we might not want to keep the search bar in the header/nav bar, I think it's actually a fine place to put it, at least on Android.

Yep, this is pretty classic in Material Design and on Android.

It's not really that foreign on iOS either, I think. Open up Safari, or Photos, and start searching -- the search box is at the top in basically an equivalent of the app bar.

Copy link
Copy Markdown
Contributor Author

@chrisbobbe chrisbobbe Mar 2, 2021

Choose a reason for hiding this comment

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

I think the thing that makes that tricky is mainly just the message content.

Ahh—right, you're right. I saw that but forgot about it when commenting.

|}>;

const componentStyles = createStyleSheet({
Expand Down Expand Up @@ -99,11 +97,12 @@ const useFetchMessages = args => {
};

export default function ChatScreen(props: Props) {
const { route, navigation } = props;
const { backgroundColor } = React.useContext(ThemeContext);

const [editMessage, setEditMessage] = React.useState<EditMessage | null>(null);

const { narrow } = props.route.params;
const { narrow, editMessage } = route.params;
const setEditMessage = (value: EditMessage | null) =>
navigation.setParams({ editMessage: value });
Copy link
Copy Markdown
Member

@gnprice gnprice Mar 2, 2021

Choose a reason for hiding this comment

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

Hmm, interesting!


const isNarrowValid = useSelector(state => getIsNarrowValid(state, narrow));

Expand All @@ -113,40 +112,35 @@ export default function ChatScreen(props: Props) {
const sayNoMessages = haveNoMessages && !isFetching;
const showComposeBox = canSendToNarrow(narrow) && !showMessagePlaceholders;

const streamColor = useSelector(state => getStreamColorForNarrow(state, narrow));

return (
<View style={[componentStyles.screen, { backgroundColor }]}>
<KeyboardAvoider style={styles.flexed} behavior="padding">
<ZulipStatusBar backgroundColor={streamColor} />
<ChatNavBar narrow={narrow} editMessage={editMessage} />
<OfflineNotice />
<UnreadNotice narrow={narrow} />
{(() => {
if (!isNarrowValid) {
return <InvalidNarrow narrow={narrow} />;
} else if (fetchError !== null) {
return <FetchError narrow={narrow} error={fetchError} />;
} else if (sayNoMessages) {
return <NoMessages narrow={narrow} />;
} else {
return (
<MessageList
narrow={narrow}
showMessagePlaceholders={showMessagePlaceholders}
startEditMessage={setEditMessage}
/>
);
}
})()}
{showComposeBox && (
<ComposeBox
narrow={narrow}
editMessage={editMessage}
completeEditMessage={() => setEditMessage(null)}
/>
)}
</KeyboardAvoider>
</View>
<KeyboardAvoider style={[componentStyles.screen, { backgroundColor }]} behavior="padding">
<ChatNavBar narrow={narrow} editMessage={editMessage} />
<OfflineNotice />
<UnreadNotice narrow={narrow} />
{(() => {
if (!isNarrowValid) {
return <InvalidNarrow narrow={narrow} />;
} else if (fetchError !== null) {
return <FetchError narrow={narrow} error={fetchError} />;
} else if (sayNoMessages) {
return <NoMessages narrow={narrow} />;
} else {
return (
<MessageList
narrow={narrow}
showMessagePlaceholders={showMessagePlaceholders}
startEditMessage={setEditMessage}
/>
);
}
})()}
{showComposeBox && (
<ComposeBox
narrow={narrow}
editMessage={editMessage}
completeEditMessage={() => setEditMessage(null)}
/>
)}
</KeyboardAvoider>
);
}
20 changes: 11 additions & 9 deletions src/common/FullScreenLoading.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,17 @@ export default function FullScreenLoading(props: Props) {
const insets = useSafeAreaInsets();

return (
<View style={componentStyles.center}>
<View
style={{
height: insets.top,
backgroundColor: BRAND_COLOR,
}}
/>
<>
<ZulipStatusBar backgroundColor={BRAND_COLOR} />
<LoadingIndicator color="black" size={80} showLogo />
</View>
<View style={componentStyles.center}>
<View
style={{
height: insets.top,
backgroundColor: BRAND_COLOR,
}}
/>
<LoadingIndicator color="black" size={80} showLogo />
</View>
</>
);
}
30 changes: 29 additions & 1 deletion src/common/KeyboardAvoider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,37 @@ type Props = $ReadOnly<{|
children: React$Node,
style?: ViewStyleProp,
contentContainerStyle?: ViewStyleProp,

/** How much the top of `KeyboardAvoider`'s layout *parent* is
* displaced downward from the top of the screen.
*
* If this isn't set correctly, the keyboard will hide some of
* the bottom of the screen (an area whose height is what this
* value should have been set to).
*
* I think `KeyboardAvoidingView`'s implementation mistakes `x`
* and `y` from `View#onLayout` to be a `View`'s position
* relative to the top left of the screen. In reality, I'm
* pretty sure they represent a `View`'s position relative to
* its parent:
* https://github.com/facebook/react-native-website/issues/2056#issuecomment-773618381
*
* But at least `KeyboardAvoidingView` exposes this prop, which
* we can use to balance the equation if we need to.
*/
keyboardVerticalOffset?: number,
|}>;

/**
* Renders RN's `KeyboardAvoidingView` on iOS, `View` on Android.
*
* This component's props that are named after
* `KeyboardAvoidingView`'s special props get passed straight through
* to that component.
*/
export default class KeyboardAvoider extends PureComponent<Props> {
render() {
const { behavior, children, style, contentContainerStyle } = this.props;
const { behavior, children, style, contentContainerStyle, keyboardVerticalOffset } = this.props;

if (Platform.OS === 'android') {
return <View style={style}>{children}</View>;
Expand All @@ -23,6 +49,8 @@ export default class KeyboardAvoider extends PureComponent<Props> {
<KeyboardAvoidingView
behavior={behavior}
contentContainerStyle={contentContainerStyle}
// See comment on this prop in the jsdoc.
keyboardVerticalOffset={keyboardVerticalOffset}
style={style}
>
{children}
Expand Down
Loading