Change stream settings UI

[juliaBichler01]

This changes the stream settings UI so that it matches the normal settings UI.

Resolves #19519

• All settings are always shown as a list
• The current setting is checked
• An admin can update the setting inline
• Everyone else can't change the settings, as the checkboxes are disabled
• Changed settings can be saved or discarded with the save/discard-widget (like in the normal settings)

As a normal user:

As an admin:

[alya]

Nice!

Let's make the following changes:

• [ ] Replace the "Retain for N days after posting" option with "Custom"
• [ ] Only show the spot for entering the time if that option is selected.
• [ ] Label the custom time like we do for message editing: "Retention period (days): [ ]"

I know we use the N days wording in main, but fixing it become a priority here, because it's so much more visible.

[alya]

We should also make sure the sections don't move up and down when the Save/Discard buttons appear/disappear.

[timabbott]

I'm pretty sure our normal settings panels don't move up/down when displaying the save/discard widgets, so we probably just need to figure out why that is and make sure the appropriate CSS applies.

[juliaBichler01]

@alya @timabbott i implemented your reviews, could you review this again?

[alya]

Thanks! Could you please:

1. Include a screenshot with a non-custom duration selected
2. Make the custom duration field width the same as in other settings (e.g. retention period); it looks too wide in the screenshots above.

Thanks!

[juliaBichler01]

@alya i added new screenshots to the pr description and changed the width of the input field.

[alya]

Looks great, thanks!

The one thing I spotted that we need to fix is how invalid input in the custom retention period field is handled, but I think that's best done by @sahil839 as a follow-up. (We'll get through all of these some day!)

[alya]

@timabbott I think you can take a look at this point.

[timabbott]

@sahil839 can you do a review/editing pass on this? I think there's a handful of things we'd need to do in order to merge this cleanly without duplication:

• Refactor the settings CSS components to just be scoped by the type of the component, not being inside the settings modal.
• Make radio elements be nicely supported by the property_changed class of function.
• Probably a bunch of stuff in the settings_org.js logic.

Expecting to do all of those as a blocker for this seems like a recipe for not being able to integrate it anytime soon.

It has a lot of code copied with apparent modifications from org_settings.js. While I would much prefer that arrange things such that this can reuse code with that system, I think it's more important to be able to integrate these design improvements for 6.0 than avoid some duplication, and I understand significant refactoring effort might be required to avoid the duplication.

It likely makes sense for you to do your own revisions to this PR as you review it, to work towards that goal and find a checkpoint you feel would be a satisfactory state to integrate (adding TODO comments on functions that should be deduplicated but seem like a bother to do so).

[sahil839]

Ok will do a review.

[sahil839]

@alya @timabbott A couple of questions before I do much changes in this -

• Can we make the retention policy setting to be a dropdown. Would particularly be helpful as we can probably reuse code for the other settings that have custom input. We already need to handle radio type setting separately (have made some progress to improve on this for org settings) and then this having custom input makes it more complex. Plus the options here are short and precise, so dropdown should be enough. If we are thinking about consistency, we would definitely be adding more settings as per #19525 and those will be based on user groups and would be dropdown only most probably.
• We can definitely keep stream post policy to be dropdown since it would anyways be changed to be a group-based setting.
• Do we need separate save-discard widget for each setting? We can keep it under one main heading for now and then can probably divide into more sections when we add more settings as per #19525.

[timabbott]

Can we make the retention policy setting to be a dropdown?

Yeah, changing it to a dropdown is totally fine, and probably a better presentation of the information.

We can definitely keep stream post policy to be dropdown since it would anyways be changed to be a group-based setting.

Yeah, this sounds correct.

Do we need separate save-discard widget for each setting?

I don't think it's required; the main thing we need to avoid it having a single section be so tall that you can't see the save/discard widget that goes with a given setting after changing its value, which could cause you to think you'd changed a setting when in fact you had not actually saved the change.

I confirmed with Alya she agrees with these points.

[juliaBichler01]

@timabbott @sahil839 should i do the mentioned changes now or should those be done in separate PRs as tim mentioned?

[sahil839]

I think you can just update the above mentioned settings to be dropdown instead of radio buttons. Then I can make an attempt to deduplicate some code and reuse settings_org.js code.

[alya]

@juliaBichler01 please consider this PR as the highest priority among the ones you're currently working on. Thanks!

[juliaBichler01]

@alya alright, i can probably start to implement the dropdown menus tomorrow evening, and hopefully finish it the day after tomorrow!

[alya]

Sounds good, thank you!

[juliaBichler01]

@alya i replaced the settings with dropdowns, sorry for the delay, this took longer than anticipated.

@sahil839 could you maybe review this again? Also, i couldn't figure out how to disable the dropdowns if the user is not allowed to edit the settings, do you know how to do this?

[sahil839]

I guess there was some misunderstanding here. I was thinking of simple select elements which we originally used inside the modal and not dropdown_list_widget.

[alya]

I guess there was some misunderstanding here. I was thinking of simple select elements which we originally used inside the modal and not dropdown_list_widget.

Sorry, what do these two elements look like?

[sahil839]

The dropdown_list_widget is the one that we use for settings like "Default language for code blocks" and for selecting bot owner in Manage bot modal where we also support search inside the dropdown menu. The select elements are the others that we use in org settings like stream creation permissions where we have options like "Admins", "Admins and moderators", etc.

[alya]

Oh, I see, I somehow missed that in the screen capture.

Yes, it should definitely be a select element!

[sahil839]

@juliaBichler01 can you update the PR as discussed above?

[juliaBichler01]

@sahil839 @alya i updated the pr to use selects instead of dropdowns, sorry that i got to it only now.

[alya]

Thanks! Let's make sure the dropdown has however much horizontal space it needs to fit the text (within the window). It's a bit short even in English, but we should make sure it looks good in any language:

[alya]

I also notice that this still needs to happen:

Replace the "Retain for N days after posting" option with "Custom"

[alya]

Can we also change the message retention policy selector in the "Create stream" UI to match the one we're implementing here?

[alya]

I also noticed and filed #23274, but that can be handled separately.

[sahil839]

Do we need separate save-discard widget for each setting?

I don't think it's required; the main thing we need to avoid it having a single section be so tall that you can't see the save/discard widget that goes with a given setting after changing its value, which could cause you to think you'd changed a setting when in fact you had not actually saved the change.

@alya @timabbott Regarding the above point, now that the settings are converted to dropdowns from radio inputs, I don't think page is long enough and all settings should be visible together for most cases (similar to a couple of sections in organization settings), so if we could just add a single save-discard widget for now and then probably have some meaningful sections once we add more stream settings as per #19525.

[alya]

we could just add a single save-discard widget for now and then probably have some meaningful sections once we add more stream settings as per https://github.com/zulip/zulip/issues/19525

Agreed!

Actually, this comment made me realize that we should make another change. At this point, it doesn't really make sense for "Who can access the stream?", "Who can post to the stream?", and "Message retention period" to be headings. We should just make them regular text. Note that these questions are already formatted as regular text in the "Create a stream" UI.

Are "Stream permissions" and "Email address" already using our regular settings heading font, or no? They should be, and I can't quite tell whether that's already the case.