Skip to content
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

Jump to bottom

Icons: Extend type-checking to the icon names, and small other cleanups. #3635

Merged
merged 4 commits into from
Jun 10, 2020
Merged

Icons: Extend type-checking to the icon names, and small other cleanups. #3635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe0688f
Icons [nfc]: Clarify the comment on IconProps; make jsdoc.
gnprice Oct 3, 2019
0d9eeae
Icons: Extend type-checking so it checks the icon names, too.
gnprice Sep 21, 2019
bfb0c73
Icons [nfc]: Rename IconType -> SpecificIconType.
gnprice Sep 21, 2019
636c57c
Icons: Cut unused icons.
gnprice Oct 1, 2019
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: 2 additions & 2 deletions src/common/FloatingActionButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import React, { PureComponent } from 'react';
import { StyleSheet, View } from 'react-native';
import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet';

import type { IconType } from './Icons';
import type { SpecificIconType } from './Icons';
import { BRAND_COLOR } from '../styles';
import Touchable from './Touchable';

Expand All @@ -20,7 +20,7 @@ type Props = $ReadOnly<{|
style?: ViewStyleProp,
disabled: boolean,
size: number,
Icon: IconType,
Icon: SpecificIconType,
onPress: () => void,
|}>;

Expand Down
108 changes: 60 additions & 48 deletions src/common/Icons.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,26 @@
import React from 'react';
import type { ComponentType } from 'react';
import type { Text } from 'react-native';
import type { Color } from 'react-native-vector-icons';
import type { Color, IconProps as IconPropsBusted } from 'react-native-vector-icons';
import Feather from 'react-native-vector-icons/Feather';
import type { FeatherGlyphs } from 'react-native-vector-icons/Feather';
import IoniconsIcon from 'react-native-vector-icons/Ionicons';
import MaterialIcon from 'react-native-vector-icons/MaterialIcons';
import SimpleLineIcons from 'react-native-vector-icons/SimpleLineIcons';

/*
* This is a reimplementation of react-native-vector-icons (RVNI)'s own
* IconProps<...> type, corresponding to the documented interface:
/**
* The props actually accepted by icon components in r-n-vector-icons.
*
* https://github.com/oblador/react-native-vector-icons/blob/v6.6.0/README.md#properties
* This corresponds to the documented interface:
* https://github.com/oblador/react-native-vector-icons/blob/v6.6.0/README.md#properties
*
* Unfortunately the upstream implementation of this type explicitly includes
* `allowFontScaling?: boolean` -- a misrespecification of the React Native
* property (which RN itself specifies to be `allowFontScaling?: ?boolean`).
* Upstream has a .js.flow file which is meant to describe this. But the
* type definition there is wrong in a couple of ways:
* * it leaves most of the properties of `Text` unspecified, and just
* defines an inexact object type so that it's much looser than reality;
* * of the handful of properties it does mention, it defines one more
* narrowly than the actual `Text`, as `allowFontScaling?: boolean` when
* it should be `allowFontScaling?: ?boolean`.
*/
type IconProps<Glyphs: string> = {|
...$Exact<$PropertyType<Text, 'props'>>,
Expand All @@ -26,47 +30,55 @@ type IconProps<Glyphs: string> = {|
color?: Color,
|};

const fixIconType = <Glyphs: string>(
iconSet: ComponentType<IconPropsBusted<Glyphs>>,
): ComponentType<IconProps<Glyphs>> => (iconSet: $FlowFixMe); // See comment above about wrong types.

/** Names acceptable for `Icon`. */
export type IconNames = FeatherGlyphs;

/** The type of an Icon whose `name` has already been specified. */
export type IconType = ComponentType<$Diff<IconProps<IconNames>, {| name: mixed |}>>;
/** A component-type for the icon set we mainly use. */
export const Icon = fixIconType<IconNames>(Feather);

/** A (type for a) component-type like `Icon` but with `name` already specified. */
export type SpecificIconType = ComponentType<$Diff<IconProps<empty>, {| name: mixed |}>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I read this as a specific (icon-type), rather than a (specific icon)'s type.

I don't think it improves legibility at any use site.

Copy link
Member Author

Choose a reason for hiding this comment

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

I read this as a specific (icon-type), rather than a (specific icon)'s type.

You've read it correctly!

This name can probably be improved. The thing that's really wrong with the existing names is the relationship between Icon and this IconType: with those two names next to each other in the same small piece of API, it sure looks like IconType should be the type of Icon, but it's not... and in fact the difference between them runs in a completely different direction from that.

One question here is: why is "Type" in the name at all? Yes this is the name of a type... but if we had a convention that types' names said "Type", we'd have a lot more names like that. I'm taking "Type" here as parallel to the relationship in React (as described with Flow) between Component and ComponentType. One of those is a class -- so already a type -- and the other is the type of a thing like Component.

So IconType would be a natural name for the type of Icon and things like it... and this isn't that, but instead is a certain variation on that. "Specific" is one attempt to gesture at that variation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm taking "Type" here as parallel to the relationship in React (as described with Flow) between Component and ComponentType.

Yes – that's the relationship it describes. It's not the type of src/common/Icons.js:Icon, but it is the type of the various React properties we have that are named Icon. (Which are thus named because they, like the Icon here, are used as React elements.)

The fact that IconType is not also the type of this Icon is infelicitous, potentially surprising, and certainly worthy of at least an explanatory comment – but it's not, I think, worth obscuring the more-widely-relevant relationship to prevent.

If we're going to rename one of them, I'd suggest instead renaming Icon to FeatherIcon. (And similarly renaming IconNames to – well, probably to FeatherGlyphs.) But that seems to address a different issue altogether, and this one only as a side-effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, renaming to FeatherIcon might be a good further cleanup. That would also help with the awkward mixture we have of places where Icon means

  • sometimes a value like IconDone or IconTerminal or IconPrivateChat, of type SpecificIconType;
  • other times the Icon export of this module, which has a different type -- it requires a name prop.

Precisely because it's an awkward mixture it's a little annoying to go in and fix, though. So I'm going to go ahead and merge this without blocking on that additional fixup.


/* $FlowFixMe: See comments above. */
export const Icon: ComponentType<IconProps<IconNames>> = Feather;
const makeIcon = <Glyphs: string>(
iconSet: ComponentType<IconPropsBusted<Glyphs>>,
name: Glyphs,
): SpecificIconType => props => {
const FixedIcon = fixIconType<Glyphs>(iconSet);
return <FixedIcon name={name} {...props} />;
};

export const IconInbox: IconType = props => <Feather name="inbox" {...props} />;
export const IconStar: IconType = props => <Feather name="star" {...props} />;
export const IconMention: IconType = props => <Feather name="at-sign" {...props} />;
export const IconSearch: IconType = props => <Feather name="search" {...props} />;
export const IconDone: IconType = props => <Feather name="check" {...props} />;
export const IconCancel: IconType = props => <Feather name="slash" {...props} />;
export const IconTrash: IconType = props => <Feather name="trash-2" {...props} />;
export const IconWarning: IconType = props => <Feather name="alert-triangle" {...props} />;
export const IconSend: IconType = props => <MaterialIcon name="send" {...props} />;
export const IconMute: IconType = props => <MaterialIcon name="volume-off" {...props} />;
export const IconStream: IconType = props => <Feather name="hash" {...props} />;
export const IconPin: IconType = props => <SimpleLineIcons name="pin" {...props} />;
export const IconPrivate: IconType = props => <Feather name="lock" {...props} />;
export const IconPrivateChat: IconType = props => <Feather name="mail" {...props} />;
export const IconDownArrow: IconType = props => <Feather name="chevron-down" {...props} />;
export const IconGoogle: IconType = props => <IoniconsIcon name="logo-google" {...props} />;
export const IconGitHub: IconType = props => <Feather name="github" {...props} />;
export const IconWindows: IconType = props => <IoniconsIcon name="logo-windows" {...props} />;
export const IconCross: IconType = props => <Feather name="x" {...props} />;
export const IconDiagnostics: IconType = props => <Feather name="activity" {...props} />;
export const IconNotifications: IconType = props => <Feather name="bell" {...props} />;
export const IconLanguage: IconType = props => <Feather name="globe" {...props} />;
export const IconNight: IconType = props => <Feather name="moon" {...props} />;
export const IconSettings: IconType = props => <Feather name="settings" {...props} />;
export const IconRight: IconType = props => <Feather name="chevron-right" {...props} />;
export const IconPlusCircle: IconType = props => <Feather name="plus-circle" {...props} />;
export const IconLeft: IconType = props => <Feather name="chevron-left" {...props} />;
export const IconPeople: IconType = props => <Feather name="users" {...props} />;
export const IconImage: IconType = props => <Feather name="image" {...props} />;
export const IconCamera: IconType = props => <Feather name="camera" {...props} />;
export const IconFile: IconType = props => <Feather name="file" {...props} />;
export const IconTerminal: IconType = props => <Feather name="terminal" {...props} />;
export const IconMoreHorizontal: IconType = props => <Feather name="more-horizontal" {...props} />;
export const IconEdit: IconType = props => <Feather name="edit" {...props} />;
export const IconPlusSquare: IconType = props => <Feather name="plus-square" {...props} />;
export const IconPlus: IconType = props => <Feather name="plus" {...props} />;
export const IconInbox = makeIcon(Feather, 'inbox');
export const IconMention = makeIcon(Feather, 'at-sign');
export const IconSearch = makeIcon(Feather, 'search');
export const IconDone = makeIcon(Feather, 'check');
Copy link
Contributor

Choose a reason for hiding this comment

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

These should still be explicitly typed, regardless of what the type's name ends up being — not for the sake of additional typechecking, but so that a human reader coming in to this file with either the type name or that of a known instance can quickly leave with the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

That'd feel pretty cluttered to me -- adding : SpecificIconType to each of these lines would feel like repetitive boilerplate.

export const IconCancel = makeIcon(Feather, 'slash');
export const IconTrash = makeIcon(Feather, 'trash-2');
export const IconSend = makeIcon(MaterialIcon, 'send');
export const IconMute = makeIcon(MaterialIcon, 'volume-off');
export const IconStream = makeIcon(Feather, 'hash');
export const IconPin = makeIcon(SimpleLineIcons, 'pin');
export const IconPrivate = makeIcon(Feather, 'lock');
export const IconPrivateChat = makeIcon(Feather, 'mail');
export const IconGoogle = makeIcon(IoniconsIcon, 'logo-google');
export const IconGitHub = makeIcon(Feather, 'github');
export const IconWindows = makeIcon(IoniconsIcon, 'logo-windows');
export const IconDiagnostics = makeIcon(Feather, 'activity');
export const IconNotifications = makeIcon(Feather, 'bell');
export const IconLanguage = makeIcon(Feather, 'globe');
export const IconNight = makeIcon(Feather, 'moon');
export const IconSettings = makeIcon(Feather, 'settings');
export const IconRight = makeIcon(Feather, 'chevron-right');
export const IconPlusCircle = makeIcon(Feather, 'plus-circle');
export const IconLeft = makeIcon(Feather, 'chevron-left');
export const IconPeople = makeIcon(Feather, 'users');
export const IconImage = makeIcon(Feather, 'image');
export const IconCamera = makeIcon(Feather, 'camera');
export const IconFile = makeIcon(Feather, 'file');
export const IconTerminal = makeIcon(Feather, 'terminal');
export const IconMoreHorizontal = makeIcon(Feather, 'more-horizontal');
export const IconEdit = makeIcon(Feather, 'edit');
export const IconPlusSquare = makeIcon(Feather, 'plus-square');
4 changes: 2 additions & 2 deletions src/common/OptionButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ import { View } from 'react-native';
import Label from './Label';
import Touchable from './Touchable';
import { IconRight } from './Icons';
import type { IconType } from './Icons';
import type { SpecificIconType } from './Icons';
import type { ThemeColors } from '../styles';
import styles, { ThemeContext } from '../styles';

type Props = $ReadOnly<{|
Icon?: IconType,
Icon?: SpecificIconType,
label: string,
onPress: () => void,
|}>;
Expand Down
4 changes: 2 additions & 2 deletions src/common/OptionRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ import React, { PureComponent } from 'react';
import { View } from 'react-native';
import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet';

import type { IconType } from './Icons';
import type { SpecificIconType } from './Icons';
import Label from './Label';
import ZulipSwitch from './ZulipSwitch';
import type { ThemeColors } from '../styles';
import styles, { ThemeContext } from '../styles';

type Props = $ReadOnly<{|
Icon?: IconType,
Icon?: SpecificIconType,
label: string,
value: boolean,
style?: ViewStyleProp,
Expand Down
4 changes: 2 additions & 2 deletions src/common/ZulipButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { StyleSheet, Text, View, ActivityIndicator } from 'react-native';
import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet';
import TranslatedText from './TranslatedText';

import type { IconType } from './Icons';
import type { SpecificIconType } from './Icons';
import { BRAND_COLOR } from '../styles';
import Touchable from './Touchable';

Expand Down Expand Up @@ -63,7 +63,7 @@ type Props = $ReadOnly<{|
style?: ViewStyleProp,
progress: boolean,
disabled: boolean,
Icon?: IconType,
Icon?: SpecificIconType,
text: string,
secondary: boolean,
onPress: () => void | Promise<void>,
Expand Down
4 changes: 2 additions & 2 deletions src/start/AuthScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type {
ApiResponseServerSettings,
} from '../types';
import { IconPrivate, IconGoogle, IconGitHub, IconWindows, IconTerminal } from '../common/Icons';
import type { IconType } from '../common/Icons';
import type { SpecificIconType } from '../common/Icons';
import { connect } from '../react-redux';
import styles from '../styles';
import { Centerer, Screen, ZulipButton } from '../common';
Expand All @@ -36,7 +36,7 @@ type AuthenticationMethodDetails = {|
/** A name to show in the UI. */
displayName: string,

Icon: IconType,
Icon: SpecificIconType,
action: 'dev' | 'password' | {| url: string |},
|};

Expand Down