EditStreamCard: Better choose when to disable stream-access options #5427
Conversation
chrisbobbe
force-pushed
the
pr-stream-access-policy
branch
from
9b6b474
to
35cc655
Compare
There was a problem hiding this comment.
Thanks @chrisbobbe! This logic looks good.
The duplication in the various disabledIfNotInitialValue expressions is pretty voluminous, though, and I think it'd be better to not duplicate -- otherwise, it's the sort of code where it'd be very easy for the different copies to diverge accidentally, with e.g. a bug being fixed in one place but not the other.
I'll push an additional commit to dedupe those. Take a look, and please go ahead and merge if it looks good to you.
src/common/SelectableOptionRow.js
Outdated
| const { text } = learnMoreButton; | ||
| switch (typeof text) { | ||
| case 'undefined': | ||
| return undefined; | ||
| case 'string': | ||
| return _(text); | ||
| default: | ||
| return _(text.text, text.values); |
There was a problem hiding this comment.
This feels like something best abstracted into a central place, with an appropriate name. I see there's a couple other examples of it above, too.
It looks like we currently don't have a handy helper for doing this unpacking of LocalizableText; mainly we just do it in the implementation of ZulipTextIntl. I'll see about doing that as a followup PR.
gnprice
force-pushed
the
pr-stream-access-policy
branch
from
35cc655
to
0dcb967
Compare
While still making it clear to admins and owners that they can change the policy, by making the "Learn more" button's text explicit about what the button can help you do. After this, the code for finding the right `message` is ready to be pulled out into a helper that simply converts a CreateWebPublicStreamPolicyT value into a user-friendly explanation. We'll do that soon.
…admins See discussion at https://chat.zulip.org/#narrow/stream/378-api-design/topic/.22Stream.20administrator.22.20for.20change.20stream.20type/near/1390982 In particular, Greg says: > For the behavior here, I think one description of a good behavior > would be that if you want to change a stream's access policy, you > need to both > > (a) have permission to edit the stream at all (which currently > means you're an org admin/owner), and > > (b) have permission to create streams with that access policy > (according to `realm_create_*_stream_policy`.)
chrisbobbe
force-pushed
the
pr-stream-access-policy
branch
from
0dcb967
to
71b6d46
Compare
|
Thanks for the review! Revision pushed. |
Ah, right. Done. |
See Greg's comment on CZO:
So, implement that behavior in the mobile UI.
Also disable the public and private options when creating a stream, since that hadn't been done yet.