StyleSheet.create and other refactors #4221

gnprice · 2020-08-07T03:37:20Z

This is the other set of followups from #4101 (comment) where I merged #4101: some small refactors to the ZulipButton component, and then that led me to some digging into StyleSheet.create and responding to an unpleasant discovery there, and to a bit of Flow metaprogramming.

(I actually still have some further changes following up on these, going deeper into tools for the Flow metaprogramming with some of the things I learned while writing this SubsetProperties. Those aren't quite ready and I'll send them separately.)

chrisbobbe

Thanks, @gnprice! Looks good except for a couple places where I'm not sure about some empty-line choices in the imports, which, as you say, were mostly modified mechanically. Once you've browsed through those and fixed any you'd like to, I'd say it's ready to merge.

chrisbobbe · 2020-08-07T22:40:11Z

src/common/Screen.js

 import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet';

 import type { ThemeData } from '../styles';
-import styles, { ThemeContext } from '../styles';
+import styles, { createStyleSheet, ThemeContext } from '../styles';
+


Is this blank line intentional?

chrisbobbe · 2020-08-07T22:42:21Z

src/streams/EditStreamCard.js


 import { Input, Label, OptionRow, ZulipButton } from '../common';
-import styles from '../styles';
+import styles, { createStyleSheet } from '../styles';
+


Added blank line

chrisbobbe · 2020-08-07T22:42:40Z

src/streams/StreamItem.js


 import type { ThemeData } from '../styles';
-import styles, { ThemeContext } from '../styles';
+import styles, { createStyleSheet, ThemeContext } from '../styles';
+


Another blank line.

chrisbobbe · 2020-08-07T22:43:38Z

src/streams/TopicItem.js

-import { StyleSheet, View } from 'react-native';
+import { View } from 'react-native';
+
+import styles, { BRAND_COLOR, createStyleSheet } from '../styles';



Added blank line

chrisbobbe · 2020-08-07T22:45:01Z

src/title/TitleGroup.js

 import { connect } from '../react-redux';
 import { UserAvatarWithPresence } from '../common';
 import { getRecipientsInGroupNarrow } from '../selectors';
-import styles from '../styles';
+
 import { navigateToAccountDetails } from '../nav/navActions';


Added blank line

chrisbobbe · 2020-08-07T22:45:16Z

src/title/TitlePrivate.js

 import { connect } from '../react-redux';
 import { Touchable, UserAvatarWithPresence, ViewPlaceholder } from '../common';
 import ActivityText from './ActivityText';
 import { getAllUsersByEmail } from '../users/userSelectors';
-import styles from '../styles';
+


Added blank line

chrisbobbe · 2020-08-07T22:45:28Z

src/title/TitleStream.js

 import { connect } from '../react-redux';
 import StreamIcon from '../streams/StreamIcon';
 import { isTopicNarrow } from '../utils/narrow';
 import { getStreamInNarrow } from '../selectors';
-import styles from '../styles';
+


Added blank line

chrisbobbe · 2020-08-07T22:45:46Z

src/user-groups/UserGroupItem.js

 import { RawLabel, Touchable } from '../common';
-import styles, { ThemeContext } from '../styles';
+


Added blank line

chrisbobbe · 2020-08-07T22:46:43Z

src/user-status/UserStatusScreen.js

 import { TranslationContext } from '../boot/TranslationProvider';
+import { createStyleSheet } from '../styles';



Could move blank line to follow 'react-native'

chrisbobbe · 2020-08-07T23:02:48Z

src/generics.js

+// Possibly this will get better after a project to fix the handling of
+// $Diff and other "type destructors", which as of 2020-08 the Flow team is
+// taking up as a priority:
+//   https://github.com/facebook/flow/issues/7886#issuecomment-669977952


So glad to see such a quick (and promising!) response from Flow! It's awesome that you wrote such a detailed comment for them to focus on.

I have a friend who's working at Facebook, and I mentioned that we use Flow. He said something like, "Whoa, actual Flow users in the wild!!"

gnprice · 2020-08-11T01:05:59Z

Thanks for the review!

Yeah, the added blank lines were not intended. I noticed a couple of them while developing the change -- they were actually introduced by eslint --fix, as when it deduplicates two imports from the same module:

import styles, { ThemeContext } from '../styles';
import { createStyleSheet } from '../styles';

->

import styles, { createStyleSheet, ThemeContext } from '../styles';

it leaves behind a blank line where the removed import was. Seems like a bug in that ESLint rule, though not a super bad one.

I'll fix those up and merge.

This makes it a bit easier to see how they relate. In particular this makes it more evident that the various specific text styles all override the `color` property on the general text style. We'll cut that always-overridden property out in the next commit.

This `color` property always gets overridden by the one from either primaryText, secondaryText, or disabledText.

See jsdoc for rationale. We'll mass-migrate all our existing uses of StyleSheet.create in the next commit.

Done mostly mechanically: $ perl -i -0pe ' s<StyleSheet.create\(> <createStyleSheet(>g || next; s<^import \{[^}]*\K( \} from \S*/styles)> <, createStyleSheet\1>ms || s<^import .*? from \S*\./\S*;\n\K> <import { createStyleSheet } from "../styles";\n>ms; s<^import \{[^\}]*\K StyleSheet,?><>ms && s<^import \{\s*\}.*?;\n><>ms ' src/**/*.js $ tools/test lint prettier --fix Plus manual fixup for a few files where those crude patterns didn't get the import right.

This rule gives a false positive in the following situation: export type * from 'foo'; export { bar } from 'foo'; (We're about to introduce an example of this in the next commit.) Fortunately it's also completely unnecessary: if there really is a double export, then Flow reports an error.

We're about to add another utility type of the same flavor as these, so it's time to make them an appropriate home. Our `types.js` is primarily about the specific types used in the app, which doesn't make the most logical home for these. We've had some in the file `react-redux.js` because that's where they're used, but they're really much more general and it's nice to have a home for them that's convenient for other modules to import from.

We'll use this shortly.

The use of specific style properties for callers to micromanage the layout of the button is pretty chaotic and fragile, introducing implicit dependencies on details of the ZulipButton implementation. It'd be good to assimilate those usages into a more coherent set of options provided by the ZulipButton interface explicitly. This would also help with making the buttons' appearance more consistent across the app. Until then, by listing here the properties we do use, we can at least make it possible when working on the ZulipButton implementation to know the scope of different ways that callers can mess with the styles.

gnprice requested a review from chrisbobbe August 7, 2020 03:37

gnprice force-pushed the pr-stylesheets branch from 161ba64 to 26cf92a Compare August 7, 2020 03:47

chrisbobbe reviewed Aug 7, 2020

View reviewed changes

gnprice added 8 commits August 10, 2020 21:36

ZulipButton [nfc]: Cut a redundant style property.

88aa458

This `color` property always gets overridden by the one from either primaryText, secondaryText, or disabledText.

styles: Add replacement for StyleSheet.create.

34ee42c

See jsdoc for rationale. We'll mass-migrate all our existing uses of StyleSheet.create in the next commit.

types: Add generic-type utility SubsetProperties.

cd36502

We'll use this shortly.

gnprice force-pushed the pr-stylesheets branch from 26cf92a to 8b30fa9 Compare August 11, 2020 04:38

gnprice merged commit 8b30fa9 into zulip:master Aug 11, 2020

gnprice deleted the pr-stylesheets branch August 11, 2020 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StyleSheet.create and other refactors #4221

StyleSheet.create and other refactors #4221

gnprice commented Aug 7, 2020

chrisbobbe left a comment •

edited

Loading

chrisbobbe Aug 7, 2020

chrisbobbe Aug 7, 2020

chrisbobbe Aug 7, 2020

chrisbobbe Aug 7, 2020

chrisbobbe Aug 7, 2020

chrisbobbe Aug 7, 2020

chrisbobbe Aug 7, 2020

chrisbobbe Aug 7, 2020

chrisbobbe Aug 7, 2020

chrisbobbe Aug 7, 2020

chrisbobbe Aug 7, 2020

gnprice commented Aug 11, 2020

		import { RawLabel, Touchable } from '../common';
		import styles, { ThemeContext } from '../styles';

		import { TranslationContext } from '../boot/TranslationProvider';
		import { createStyleSheet } from '../styles';

StyleSheet.create and other refactors #4221

StyleSheet.create and other refactors #4221

Conversation

gnprice commented Aug 7, 2020

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Aug 11, 2020

chrisbobbe left a comment •

edited

Loading