Topic edit modal #5505
Topic edit modal #5505
Conversation
Screen.Recording.2022-09-24.at.6.50.03.PM.mov |
|
I think we also want the Edit Topic button to only appear for admins for now. I can borrow this: if (roleIsAtLeast(ownUserRole, Role.Admin)) {
buttons.push(deleteTopic);
} |
dootMaster
force-pushed
the
topicEditModal
branch
2 times, most recently
from
3b81cb0 to
ec60c8d
Compare
dootMaster
force-pushed
the
topicEditModal
branch
from
ec60c8d to
658c15f
Compare
That sounds right; please do borrow that! 🙂 Since it won't be obvious to everyone reading that code, please include a code comment saying why we're only showing it to admins-and-above, linking to discussion if any. |
There was a problem hiding this comment.
Thanks, @dootMaster! See comments below.
For the branch structure, I think it makes sense to have one NFC commit that converts SearchMessagesCard to a function component, followed by one non-NFC commit whose "single idea" is something like "add a new edit-topic UI" and contains all the work that accomplishes that. The added code is pretty coherent and is all meant to work together, so I think it's easier to see how it all works together when it's added in the same commit. Also please remember that all commits should pass all tests, and feel free to ask in #git help if you have questions on fixing/tidying commits. 🙂
src/boot/TopicModalProvider.js
Outdated
| }); | ||
|
|
||
| const startEditTopic = useCallback( | ||
| async (streamId, topic, streamsById, _) => { |
There was a problem hiding this comment.
Let's just ask the caller for streamId and topic. Those are the interesting things that make this edit-topic session different from another one: "Which topic do we want to edit? This topic, in this stream."
streamsById and _ are useful bits of background data that we end up consulting, but they're not special knowledge about the requested UI interaction that the caller has to provide. We can get them by calling useSelector and useContext, respectively, above this in TopicModalProvider. That way, this component will rerender when we have something new for those, which is helpful for avoiding bugs like (with _) showing text with the UI language setting at the time the modal was opened, which might be different from the current language setting.
There was a problem hiding this comment.
Also, let's have startEditTopic bail out (return) early if there's already an edit-topic interaction in progress, so we don't have two interactions stepping on each other and corrupting each other's state.
| const styles = createStyleSheet({ | ||
| wrapper: { | ||
| flex: 1, | ||
| justifyContent: 'center', | ||
| alignItems: 'center', | ||
| }, | ||
| modalView: { | ||
| margin: 10, | ||
| alignItems: 'center', | ||
| backgroundColor, | ||
| padding: 20, | ||
| borderStyle: 'solid', | ||
| borderWidth: 1, | ||
| borderColor: 'gray', | ||
| borderRadius: 20, | ||
| width: '90%', | ||
| }, | ||
| buttonContainer: { | ||
| display: 'flex', | ||
| flexDirection: 'row', | ||
| justifyContent: 'space-between', | ||
| width: '60%', | ||
| }, | ||
| input: { | ||
| width: '90%', | ||
| borderWidth: 1, | ||
| borderRadius: 5, | ||
| marginBottom: 16, | ||
| ...inputMarginPadding, | ||
| backgroundColor: 'white', | ||
| borderStyle: 'solid', | ||
| borderColor: 'black', | ||
| }, | ||
| button: { | ||
| position: 'relative', | ||
| justifyContent: 'center', | ||
| alignItems: 'center', | ||
| borderRadius: 32, | ||
| padding: 8, | ||
| }, | ||
| titleText: { | ||
| fontSize: 18, | ||
| lineHeight: 21, | ||
| fontWeight: 'bold', | ||
| color: BRAND_COLOR, | ||
| marginBottom: 12, | ||
| }, | ||
| text: { | ||
| fontSize: 14, | ||
| lineHeight: 21, | ||
| fontWeight: 'bold', | ||
| }, | ||
| submitButtonText: { | ||
| color: 'white', | ||
| }, | ||
| cancelButtonText: { | ||
| color: BRAND_COLOR, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
That's a lot of style attributes! 🙂
I think we can probably improve the appearance of the modal, and also need fewer of these ad-hoc style attributes, by using our components ZulipTextButton and Input for the buttons and text input.
These are meant to be reusable and to give a consistent look-and-feel to the app, with an eye on Material Design. The Material Design page on dialogs looks like a good resource that might be helpful in deciding how we want this modal to look.
|
@chrisbobbe Thanks for reviewing! How is this check achieved?
I've modified it to check const startEditTopic = useCallback(
async (streamId: number, topic: string) => {
const { visible } = topicModalProviderState;
if (visible) {
return;
}
setTopicModalProviderState({
visible: true,
streamId,
topic,
});
}, [topicModalProviderState]);
``` |
We'll use this for zulip#5306; see the plan in discussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5306.20Follow.20.2Fnear.2F.20links.20through.20topic.20moves.2Frenames/near/1407930 In particular, we want the stream and topic for a stream message that might not be in our data structures. We'll use this endpoint to fetch that information. topic edit modal [nfc]: Add TopicModalProvider context component. Contains visibility context and handler callback context. Sets up context for modal handler to be called inside topic action sheets. topic edit modal [nfc]: Provide topic modal Context hook to children. The useTopicModalHandler is called normally in TopicItem and TitleStream. In order to deliver the callbacks to the action sheets in MessageList, the context hook is called in ChatScreen and a bit of prop-drilling is performed. topic edit modal: Add translation for action sheet button. topic edit modal: Add modal and functionality to perform topic name updates. Fixes zulip#5365 topid edit modal [nfc]: Revise Flow types for relevant components. topic edit modal: Modify webview unit tests to accommodate feature update.
dootMaster
force-pushed
the
topicEditModal
branch
from
658c15f to
2b49955
Compare
|
(For the record, the next revision of this PR went to #5510.) |
This draft PR is ready for review. What should a unit test for this feature entail?