-
-
Notifications
You must be signed in to change notification settings - Fork 651
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
StreamSettingsScreen: s/Private/Privacy/; use radio buttons, not a switch #5369
Conversation
Before/after: Apr-28-2022.13-16-55.mp4Apr-28-2022.13-15-25.mp4 |
Cool, looks good! When we are adding all the privacy options, I think we should try including more detailed description text to each option, like we do in the web app. |
Yeah, I think so too. #5360 is meant to help the appearance when we have explanatory text that's multiple lines long, which is a case we haven't had before with that UI component. |
fc9cba4
to
aef1f02
Compare
Thanks for the review, Alya! I've just rebased, and I'll mark this as non-draft for when Greg gets back from vacation. |
aef1f02
to
9b9b90d
Compare
Rebased. #5360 has been merged, making this branch just one commit. 🙂 (Though I acknowledge it's kind of a big diff.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chrisbobbe! This code looks great -- the design makes sense, and the code is generally quite clean. A few comments below.
src/common/InputRowRadioButtons.js
Outdated
title: string, | ||
subtitle?: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two should be strings waiting to be translated, right? I.e. not strings we're going to show literally to the user without passing through translation.
If so, it'd be good to make that clear in the code (and if not, to make the opposite clear). In particular, it's good for it to be clear where in the code the responsibility lies for invoking translation, so that we don't end up either failing to translate or attempting to double-translate.
I think a pretty good immediate way to do that would be to make the type LocalizableText
. That's at least as good as any comment, plus it will prevent trying to use the string directly without translating it.
I don't think we currently have a way to ensure that a string not meant for translation -- e.g. one that's already been through translation -- accidentally gets translated. Now that I think about it, it'd probably be straightforward to do so by making LocalizableText
an opaque type. But that's a project for another day.
src/common/InputRowRadioButtons.js
Outdated
/** What the setting is about, e.g., "Theme". */ | ||
label: string, | ||
|
||
/** What the setting is about, in a short sentence or two. */ | ||
description?: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment as for the title and subtitle above.
src/common/InputRowRadioButtons.js
Outdated
const { navigation, label, description, valueKey, items, onValueChange } = props; | ||
|
||
const screenKey = useRef( | ||
`selectable-options-${Math.floor(Math.random() * Number.MAX_SAFE_INTEGER).toString(36)}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Probably this should be a helper function with a name, for the sake of both brevity and clarity. Which basically means take the existing eg.randString
and put it somewhere in the non-test code.
src/common/InputRowRadioButtons.js
Outdated
// TODO: Can use .push instead of .navigate? See if Flow types are wrong | ||
// in saying we can't pass `key` when using .push. | ||
navigation.navigate({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, curious.
Even if push
won't accept a key
, the behavior is fine, fortunately. I don't recall whether navigate
's funky rewind-history behavior (in stack navigators) takes account of key
to distinguish the desired new destination from existing routes on the stack -- but even if it doesn't, SelectableOptionsScreen
is not going to push anything else on the stack. That means it'll never be somewhere earlier in the stack when we're here on whatever screen this InputRowRadioButtons
appears on; so that funky rewinding won't ever trigger here; and the behavior of navigate
will be exactly the same as that of push
would have been.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I'll make a note that using .navigate
is fine.
<Touchable onPress={handleRowPressed}> | ||
<View style={styles.wrapper}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Touchable
itself already creates a wrapper View. (This smooths over some variation between the underlying primitives RN gives us on iOS vs. Android; see the Touchable
implementation.)
Can we use that one instead of having a second wrapper inside it? That'd look like:
<Touchable onPress={handleRowPressed}> | |
<View style={styles.wrapper}> | |
<Touchable onPress={handleRowPressed} style={styles.wrapper}> |
(and drop a </View>
at the end, and reindent.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see. I guess we even have that in our jsdoc for Touchable
:
* @prop [children] - A single component (not zero, or more than one.)
So be it, then.
src/common/InputRowRadioButtons.js
Outdated
// Live-update the selectable-options screen. | ||
useEffect(() => { | ||
navigation.dispatch( | ||
state => | ||
state.routes.find(route => route.key === screenKey) | ||
? { ...CommonActions.setParams(screenParams), source: screenKey } | ||
: CommonActions.reset(state), // no-op |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, funky. I'm reading about the relevant part of the React Nav API here:
https://reactnavigation.org/docs/5.x/navigation-prop#dispatch
https://reactnavigation.org/docs/5.x/navigation-actions/#setparams
Do we need the conditional and the no-op case? What happens if we just say:
// Live-update the selectable-options screen. | |
useEffect(() => { | |
navigation.dispatch( | |
state => | |
state.routes.find(route => route.key === screenKey) | |
? { ...CommonActions.setParams(screenParams), source: screenKey } | |
: CommonActions.reset(state), // no-op | |
// Live-update the selectable-options screen. | |
useEffect(() => { | |
navigation.dispatch( | |
{ ...CommonActions.setParams(screenParams), source: screenKey }, |
; does that blow up if the route key passed as source
isn't actually found on any route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good thought—whatever we end up with, I should probably add a code comment. On pressing the "Edit stream" button, which brings me to EditStreamScreen
, I get this:
It's dismissable and probably harmless? Glad to have the dev-only warning, I guess, rather than a silent failure, to help debug when one does expect navigation.dispatch
to do something visible and instead nothing happens. I could do a LogBox
suppression so that we don't even see this particular warning in dev.
We can set onUnhandledAction
on the navigation container, to set the behavior: https://reactnavigation.org/docs/navigation-container#onunhandledaction . That doc is for 6.x. The doc for 5.x, which we're on, doesn't mention the prop, but I'm seeing its implementation in node_modules
. By default, it looks like nothing happens in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I think given that it's an error in dev, and it's not hard to write the code to avoid it, it's best to avoid it.
// This param is a function, so React Nav is right to point out that | ||
// it isn't serializable. But this is fine as long as we don't try to | ||
// persist navigation state for this screen or set up deep linking to | ||
// it, hence the LogBox suppression below. | ||
// | ||
// React Navigation doesn't offer a more sensible way to have us pass | ||
// the selection to the calling screen. …We could store the selection | ||
// as a route param on the calling screen, or in Redux. But from this | ||
// screen's perspective, that's basically just setting a global | ||
// variable. Better to offer this explicit, side-effect-free way for | ||
// the data to flow where it should, when it should. | ||
onRequestSelectionChange: (itemKey: TItemKey, requestedValue: boolean) => void, | ||
|}, | ||
>, | ||
|}>; | ||
|
||
// React Navigation would give us a console warning about non-serializable | ||
// route params. For more about the warning, see | ||
// https://reactnavigation.org/docs/5.x/troubleshooting/#i-get-the-warning-non-serializable-values-were-found-in-the-navigation-state | ||
// See comment on this param, above. | ||
LogBox.ignoreLogs([/selectable-options > params\.onRequestSelectionChange \(Function\)/]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all makes sense! Thanks for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's basically copied from EmojiPickerScreen
. 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, OK -- I thought it sounded a bit familiar, but figured I must have looked at a version of this PR earlier.
Still, that's also text that you wrote, so the thanks aren't wrong 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning was yours, though, as I remember! We can share the thanks, then? 😅
9b9b90d
to
5ceec85
Compare
Thanks for the review! Revision pushed. |
{ key: 'public', title: 'Public' }, | ||
{ key: 'private', title: 'Private' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong in the office today saying 'Public' and 'Private' aren't in messages_en.json; they are. 🙂
These functions are useful for test code, but they'd also be useful for non-test code. So, move them out of test-only files.
5ceec85
to
f39ba15
Compare
Another revision pushed, with some things we discussed in the office yesterday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revision! All looks good except one bug in the SelectableOptionRow translation refactor.
src/settings/LanguagePicker.js
Outdated
getFilteredLanguageList: string => Language[] = filter => { | ||
const list = this.getTranslatedLanguages(); | ||
|
||
getFilteredLanguageList: string => $ReadOnlyArray<Language> = filter => { | ||
if (!filter) { | ||
return list; | ||
return languages; | ||
} | ||
|
||
return list.filter(item => { | ||
return languages.filter(item => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't seem right -- it means that the search will look at the English name and selfname, instead of the current UI language's name and selfname.
The code around here could be improved in general, but probably the best thing for the current task is to keep the change to this code simple and local: just handle subtitle=
below in the same way as title=
, without changing these other functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed! Thanks for catching this.
…itch For zulip#5250, we'll need to let the user choose one among more than two options. That'll call for a radio-button-style input, rather than a switch. So, make a new component InputRowRadioButtons, and use it for an input labeled "Privacy", replacing the SwitchRow input labeled "Private". And, to support that component, write and wire up another new component, SelectableOptionsScreen.
f39ba15
to
e720924
Compare
Thanks for the review! Revision pushed. |
Thanks! Looks good; merging. |
This includes my current revision for #5360, which we hope to merge soon.Done!For #5250, we'll need to let the user choose one among more than two
options. That'll call for a radio-button-style input, rather than a
switch.
So, make a new component InputRowRadioButtons, and use it for an
input labeled "Privacy", replacing the SwitchRow input labeled
"Private".
And, to support that component, write and wire up a new
SelectableOptionsScreen.