Page MenuHomePhabricator
Differential D3702

[web] Pull colors in `ColorSelector` from `selectedThreadColors`
ClosedPublic
Actions

Authored by atul on Apr 11 2022, 12:44 PM.
Tags
None
Referenced Files
F5073252: D3702.diff
Fri, Mar 28, 7:04 AM
Unknown Object (File)
Tue, Mar 25, 1:09 AM
Unknown Object (File)
Sat, Mar 22, 9:09 PM
Unknown Object (File)
Fri, Mar 14, 2:35 AM
Unknown Object (File)
Fri, Mar 14, 2:35 AM
Unknown Object (File)
Fri, Mar 14, 2:35 AM
Unknown Object (File)
Fri, Mar 14, 2:35 AM
Unknown Object (File)
Fri, Mar 14, 2:34 AM
View All Files
Subscribers
• abosh
• adrian
ashoat
• benschac
• karol

Details

Reviewers
• jacek
tomek
Commits
rCOMM97fa0a4fb3d6: [web] Pull colors in `ColorSelector` from `selectedThreadColors`
Summary

Slightly better than it was before where we were hardcoding these values twice. Will create the ColorSelectorButton components in a for loop instead of copy/pasting 10 times in subsequent diff.

Depends on D3701

Test Plan

Continues to work as before, colors show up correctly:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul created this revision.Apr 11 2022, 12:44 PM
Herald added subscribers: • abosh, • benschac, • karol and 2 others. · View Herald TranscriptApr 11 2022, 12:44 PM
atul edited the test plan for this revision. (Show Details)Apr 11 2022, 12:44 PM
Harbormaster completed remote builds in B8092: Diff 11317.Apr 11 2022, 12:51 PM
atul requested review of this revision.Apr 11 2022, 12:51 PM
atul edited the summary of this revision. (Show Details)Apr 11 2022, 2:59 PM
atul added a parent revision: D3701: [web] Use `tinycolor.equals(a, b)` in `ColorSelectorButton`.
atul added a child revision: D3703: [web] Use `flex-wrap` to lay out `ColorSelectorButton`s in `ColorSelector`.
tomek accepted this revision.Apr 12 2022, 3:38 AM
tomek added inline comments.
lib/shared/thread-utils.js
98–100 ↗(On Diff #11317)

Looking at https://stackoverflow.com/questions/5525795/does-javascript-guarantee-object-property-order/38218582#38218582 it seems like iteration order is a complicated topic. This implementation is probably fine, but I'd feel safer to sort these based on the value from the object.

This revision is now accepted and ready to land.Apr 12 2022, 3:38 AM
• benschac added inline comments.Apr 12 2022, 9:13 AM
web/modals/threads/color-selector.react.js
21 ↗(On Diff #11317)

Sorry, I should have been more direct.

example:

<ColorSelectorButton color="blue"

inside ColorSelector we access the hex value: selectedThreadColors[props.color] which would return the hex value.

selectedThreadColors[0] doesn't tell me what color this color selector is.

atul marked an inline comment as done.Apr 12 2022, 11:34 AM
atul added inline comments.
lib/shared/thread-utils.js
98–100 ↗(On Diff #11317)

Thanks for catching that! It looks like the color aren't appearing in the order intended. I naively assumed that the order was preserved.

It looks like one way we can approach this is something like:

const selectedThreadColors = Object.entries(selectedThreadColorsObj).sort((a, b) => a[1] - b[1]).map((each) => each[0]);

Inspired by https://stackoverflow.com/a/37607084

I'll update this diff with the change

atul marked an inline comment as done.Apr 12 2022, 11:37 AM
atul added inline comments.
lib/shared/thread-utils.js
98–100 ↗(On Diff #11317)

Actually, I'll make this a new diff since it seems like there's a few ways to approach this and there may be some discussion. Also to unblock and land the rest of the accepted stack.

atul updated this revision to Diff 11365.Apr 12 2022, 11:41 AM

rebase before landing

Harbormaster completed remote builds in B8132: Diff 11365.Apr 12 2022, 11:46 AM
Closed by commit rCOMM97fa0a4fb3d6: [web] Pull colors in `ColorSelector` from `selectedThreadColors` (authored by atul). · Explain WhyApr 12 2022, 11:47 AM
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM97fa0a4fb3d6: [web] Pull colors in `ColorSelector` from `selectedThreadColors`.
atul removed a parent revision: D3701: [web] Use `tinycolor.equals(a, b)` in `ColorSelectorButton`.Apr 12 2022, 12:31 PM
atul added inline comments.Apr 12 2022, 12:47 PM
web/modals/threads/color-selector.react.js
21 ↗(On Diff #11317)

That's a fair point, but the colors were chosen arbitrarily and don't necessarily map cleanly to "English" names.

I agree that colors should usually be labeled, but it'd be difficult in this case. Open to any ideas though

atul mentioned this in D3715: [web] Modify `selectedThreadColors` to preserve order of keys.Apr 12 2022, 3:57 PM
tomek added inline comments.Apr 13 2022, 12:31 AM
lib/shared/thread-utils.js
98–100 ↗(On Diff #11317)

That makes sense, thanks!

atul mentioned this in rCOMMfad6a638b0f9: [web] Modify `selectedThreadColors` to preserve order of keys.Apr 14 2022, 3:54 PM

Revision Contents
Changeset List

PathSize
lib/
shared/
5 lines
web/
modals/
threads/
22 lines

Diff 11366

View Options

lib/shared/thread-utils.js

Show All 20 Lines
import Button from '../../../components/button.react'; import Button from '../../../components/button.react';
import EnumSettingsOption from '../../../components/enum-settings-option.react'; import EnumSettingsOption from '../../../components/enum-settings-option.react';
import { useSelector } from '../../../redux/redux-utils'; import { useSelector } from '../../../redux/redux-utils';
import Modal from '../../modal.react'; import Modal from '../../modal.react';
import css from './notifications-modal.css'; import css from './notifications-modal.css';
type NotificationSettings = 'focused' | 'badge-only' | 'background'; type NotificationSettings = 'focused' | 'badge-only' | 'background';
const BANNER_NOTIFS = 'Banner notifs';
const BADGE_COUNT = 'Badge count';
const IN_FOCUSED_TAB = 'Lives in Focused tab';
const IN_BACKGROUND_TAB = 'Lives in Background tab';
const focusedStatements = [
{ statement: BANNER_NOTIFS, isStatementValid: true },
{ statement: BADGE_COUNT, isStatementValid: true },
{ statement: IN_FOCUSED_TAB, isStatementValid: true },
];
const badgeOnlyStatements = [
{ statement: BANNER_NOTIFS, isStatementValid: false },
{ statement: BADGE_COUNT, isStatementValid: true },
{ statement: IN_FOCUSED_TAB, isStatementValid: true },
];
const backgroundStatements = [
{ statement: BANNER_NOTIFS, isStatementValid: false },
{ statement: BADGE_COUNT, isStatementValid: false },
{ statement: IN_BACKGROUND_TAB, isStatementValid: true },
];
type Props = { type Props = {
+threadID: string, +threadID: string,
+onClose: () => void, +onClose: () => void,
}; };
function NotificationsModal(props: Props): React.Node { function NotificationsModal(props: Props): React.Node {
const { onClose, threadID } = props; const { onClose, threadID } = props;
const threadInfo = useSelector(state => threadInfoSelector(state)[threadID]); const threadInfo = useSelector(state => threadInfoSelector(state)[threadID]);
const { subscription } = threadInfo.currentUser; const { subscription } = threadInfo.currentUser;
const initialThreadSetting = React.useMemo<NotificationSettings>(() => { const initialThreadSetting = React.useMemo<NotificationSettings>(() => {
if (!subscription.home) { if (!subscription.home) {
return 'background'; return 'background';
Show All 19 Linesfunction NotificationsModal(props: Props): React.Node {
); );
const onBackgroundSelected = React.useCallback( const onBackgroundSelected = React.useCallback(
() => setNotificationSettings('background'), () => setNotificationSettings('background'),
[], [],
); );
const isFocusedSelected = notificationSettings === 'focused'; const isFocusedSelected = notificationSettings === 'focused';
const focusedItem = React.useMemo(() => { const focusedItem = React.useMemo(() => {
const statements = [
{ statement: 'Banner notifs', isStatementValid: true },
{ statement: 'Badge count', isStatementValid: true },
{ statement: 'Lives in Focused tab', isStatementValid: true },
];
const icon = ( const icon = (
<img <img
src={`${assetCacheURLPrefix}/${focusedNotificationsIllustrationAsset.fileName}`} src={`${assetCacheURLPrefix}/${focusedNotificationsIllustrationAsset.fileName}`}
height={focusedNotificationsIllustrationAsset.height} height={focusedNotificationsIllustrationAsset.height}
width={focusedNotificationsIllustrationAsset.width} width={focusedNotificationsIllustrationAsset.width}
/> />
); );
return ( return (
<EnumSettingsOption <EnumSettingsOption
selected={isFocusedSelected} selected={isFocusedSelected}
title="Focused (enabled)" title="Focused (enabled)"
statements={statements} statements={focusedStatements}
icon={icon} icon={icon}
onSelect={onFocusedSelected} onSelect={onFocusedSelected}
/> />
); );
}, [isFocusedSelected, onFocusedSelected]); }, [isFocusedSelected, onFocusedSelected]);
const isFocusedBadgeOnlySelected = notificationSettings === 'badge-only'; const isFocusedBadgeOnlySelected = notificationSettings === 'badge-only';
const focusedBadgeOnlyItem = React.useMemo(() => { const focusedBadgeOnlyItem = React.useMemo(() => {
const statements = [
{ statement: 'Banner notifs', isStatementValid: false },
{ statement: 'Badge count', isStatementValid: true },
{ statement: 'Lives in Focused tab', isStatementValid: true },
];
const icon = ( const icon = (
<img <img
src={`${assetCacheURLPrefix}/${badgeOnlyNotificationsIllustrationAsset.fileName}`} src={`${assetCacheURLPrefix}/${badgeOnlyNotificationsIllustrationAsset.fileName}`}
height={badgeOnlyNotificationsIllustrationAsset.height} height={badgeOnlyNotificationsIllustrationAsset.height}
width={badgeOnlyNotificationsIllustrationAsset.width} width={badgeOnlyNotificationsIllustrationAsset.width}
/> />
); );
return ( return (
<EnumSettingsOption <EnumSettingsOption
selected={isFocusedBadgeOnlySelected} selected={isFocusedBadgeOnlySelected}
title="Focused (badge only)" title="Focused (badge only)"
statements={statements} statements={badgeOnlyStatements}
icon={icon} icon={icon}
onSelect={onBadgeOnlySelected} onSelect={onBadgeOnlySelected}
/> />
); );
}, [isFocusedBadgeOnlySelected, onBadgeOnlySelected]); }, [isFocusedBadgeOnlySelected, onBadgeOnlySelected]);
const isBackgroundSelected = notificationSettings === 'background'; const isBackgroundSelected = notificationSettings === 'background';
const backgroundItem = React.useMemo(() => { const backgroundItem = React.useMemo(() => {
const statements = [
{ statement: 'Banner notifs', isStatementValid: false },
{ statement: 'Badge count', isStatementValid: false },
{ statement: 'Lives in Background tab', isStatementValid: true },
];
const icon = ( const icon = (
<img <img
src={`${assetCacheURLPrefix}/${backgroundNotificationsIllustrationAsset.fileName}`} src={`${assetCacheURLPrefix}/${backgroundNotificationsIllustrationAsset.fileName}`}
height={backgroundNotificationsIllustrationAsset.height} height={backgroundNotificationsIllustrationAsset.height}
width={backgroundNotificationsIllustrationAsset.width} width={backgroundNotificationsIllustrationAsset.width}
/> />
); );
return ( return (
<EnumSettingsOption <EnumSettingsOption
selected={isBackgroundSelected} selected={isBackgroundSelected}
title="Background" title="Background"
statements={statements} statements={backgroundStatements}
icon={icon} icon={icon}
onSelect={onBackgroundSelected} onSelect={onBackgroundSelected}
/> />
); );
}, [isBackgroundSelected, onBackgroundSelected]); }, [isBackgroundSelected, onBackgroundSelected]);
const dispatchActionPromise = useDispatchActionPromise(); const dispatchActionPromise = useDispatchActionPromise();
▲ Show 20 Lines • Show All 43 Lines • Show Last 20 Lines
View Options

web/modals/threads/color-selector.react.js

Show All 20 Lines
import Button from '../../../components/button.react'; import Button from '../../../components/button.react';
import EnumSettingsOption from '../../../components/enum-settings-option.react'; import EnumSettingsOption from '../../../components/enum-settings-option.react';
import { useSelector } from '../../../redux/redux-utils'; import { useSelector } from '../../../redux/redux-utils';
import Modal from '../../modal.react'; import Modal from '../../modal.react';
import css from './notifications-modal.css'; import css from './notifications-modal.css';
type NotificationSettings = 'focused' | 'badge-only' | 'background'; type NotificationSettings = 'focused' | 'badge-only' | 'background';
const BANNER_NOTIFS = 'Banner notifs';
const BADGE_COUNT = 'Badge count';
const IN_FOCUSED_TAB = 'Lives in Focused tab';
const IN_BACKGROUND_TAB = 'Lives in Background tab';
const focusedStatements = [
{ statement: BANNER_NOTIFS, isStatementValid: true },
{ statement: BADGE_COUNT, isStatementValid: true },
{ statement: IN_FOCUSED_TAB, isStatementValid: true },
];
const badgeOnlyStatements = [
{ statement: BANNER_NOTIFS, isStatementValid: false },
{ statement: BADGE_COUNT, isStatementValid: true },
{ statement: IN_FOCUSED_TAB, isStatementValid: true },
];
const backgroundStatements = [
{ statement: BANNER_NOTIFS, isStatementValid: false },
{ statement: BADGE_COUNT, isStatementValid: false },
{ statement: IN_BACKGROUND_TAB, isStatementValid: true },
];
type Props = { type Props = {
+threadID: string, +threadID: string,
+onClose: () => void, +onClose: () => void,
}; };
function NotificationsModal(props: Props): React.Node { function NotificationsModal(props: Props): React.Node {
const { onClose, threadID } = props; const { onClose, threadID } = props;
const threadInfo = useSelector(state => threadInfoSelector(state)[threadID]); const threadInfo = useSelector(state => threadInfoSelector(state)[threadID]);
const { subscription } = threadInfo.currentUser; const { subscription } = threadInfo.currentUser;
const initialThreadSetting = React.useMemo<NotificationSettings>(() => { const initialThreadSetting = React.useMemo<NotificationSettings>(() => {
if (!subscription.home) { if (!subscription.home) {
return 'background'; return 'background';
Show All 19 Linesfunction NotificationsModal(props: Props): React.Node {
); );
const onBackgroundSelected = React.useCallback( const onBackgroundSelected = React.useCallback(
() => setNotificationSettings('background'), () => setNotificationSettings('background'),
[], [],
); );
const isFocusedSelected = notificationSettings === 'focused'; const isFocusedSelected = notificationSettings === 'focused';
const focusedItem = React.useMemo(() => { const focusedItem = React.useMemo(() => {
const statements = [
{ statement: 'Banner notifs', isStatementValid: true },
{ statement: 'Badge count', isStatementValid: true },
{ statement: 'Lives in Focused tab', isStatementValid: true },
];
const icon = ( const icon = (
<img <img
src={`${assetCacheURLPrefix}/${focusedNotificationsIllustrationAsset.fileName}`} src={`${assetCacheURLPrefix}/${focusedNotificationsIllustrationAsset.fileName}`}
height={focusedNotificationsIllustrationAsset.height} height={focusedNotificationsIllustrationAsset.height}
width={focusedNotificationsIllustrationAsset.width} width={focusedNotificationsIllustrationAsset.width}
/> />
); );
return ( return (
<EnumSettingsOption <EnumSettingsOption
selected={isFocusedSelected} selected={isFocusedSelected}
title="Focused (enabled)" title="Focused (enabled)"
statements={statements} statements={focusedStatements}
icon={icon} icon={icon}
onSelect={onFocusedSelected} onSelect={onFocusedSelected}
/> />
); );
}, [isFocusedSelected, onFocusedSelected]); }, [isFocusedSelected, onFocusedSelected]);
const isFocusedBadgeOnlySelected = notificationSettings === 'badge-only'; const isFocusedBadgeOnlySelected = notificationSettings === 'badge-only';
const focusedBadgeOnlyItem = React.useMemo(() => { const focusedBadgeOnlyItem = React.useMemo(() => {
const statements = [
{ statement: 'Banner notifs', isStatementValid: false },
{ statement: 'Badge count', isStatementValid: true },
{ statement: 'Lives in Focused tab', isStatementValid: true },
];
const icon = ( const icon = (
<img <img
src={`${assetCacheURLPrefix}/${badgeOnlyNotificationsIllustrationAsset.fileName}`} src={`${assetCacheURLPrefix}/${badgeOnlyNotificationsIllustrationAsset.fileName}`}
height={badgeOnlyNotificationsIllustrationAsset.height} height={badgeOnlyNotificationsIllustrationAsset.height}
width={badgeOnlyNotificationsIllustrationAsset.width} width={badgeOnlyNotificationsIllustrationAsset.width}
/> />
); );
return ( return (
<EnumSettingsOption <EnumSettingsOption
selected={isFocusedBadgeOnlySelected} selected={isFocusedBadgeOnlySelected}
title="Focused (badge only)" title="Focused (badge only)"
statements={statements} statements={badgeOnlyStatements}
icon={icon} icon={icon}
onSelect={onBadgeOnlySelected} onSelect={onBadgeOnlySelected}
/> />
); );
}, [isFocusedBadgeOnlySelected, onBadgeOnlySelected]); }, [isFocusedBadgeOnlySelected, onBadgeOnlySelected]);
const isBackgroundSelected = notificationSettings === 'background'; const isBackgroundSelected = notificationSettings === 'background';
const backgroundItem = React.useMemo(() => { const backgroundItem = React.useMemo(() => {
const statements = [
{ statement: 'Banner notifs', isStatementValid: false },
{ statement: 'Badge count', isStatementValid: false },
{ statement: 'Lives in Background tab', isStatementValid: true },
];
const icon = ( const icon = (
<img <img
src={`${assetCacheURLPrefix}/${backgroundNotificationsIllustrationAsset.fileName}`} src={`${assetCacheURLPrefix}/${backgroundNotificationsIllustrationAsset.fileName}`}
height={backgroundNotificationsIllustrationAsset.height} height={backgroundNotificationsIllustrationAsset.height}
width={backgroundNotificationsIllustrationAsset.width} width={backgroundNotificationsIllustrationAsset.width}
/> />
); );
return ( return (
<EnumSettingsOption <EnumSettingsOption
selected={isBackgroundSelected} selected={isBackgroundSelected}
title="Background" title="Background"
statements={statements} statements={backgroundStatements}
icon={icon} icon={icon}
onSelect={onBackgroundSelected} onSelect={onBackgroundSelected}
/> />
); );
}, [isBackgroundSelected, onBackgroundSelected]); }, [isBackgroundSelected, onBackgroundSelected]);
const dispatchActionPromise = useDispatchActionPromise(); const dispatchActionPromise = useDispatchActionPromise();
▲ Show 20 Lines • Show All 43 Lines • Show Last 20 Lines