-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
docs: Update new feature tutorial, add emoji flag #23527
base: main
Are you sure you want to change the base?
Conversation
@timabbott Will you be reviewing this one? |
I think @laurynmm should review first; she's worked on the tutorial more recently than I have. |
I can definitely review this, but it likely won't be until Monday. Before then, @erwsmith could you fix the commits to follow our commit discipline guide? This doc on examining and tidying your commit history might also be helpful. Thanks and let me know if you have any questions that the documentation doesn't cover. |
Sounds good, thanks @laurynmm! Thank you for the links, I read through them and took a crack at tidying up my commit history, and I think I just made things worse as I haven't been able to rebase at all without creating merge conflicts and I'm caught up in a git vortex that I'm not sure how to escape, I'll keep reading and trying to get things fixed, but perhaps you can help walk me through some of the specific issues you noticed with my commits and maybe help me clean things up on Monday depending on how far I get. |
9bc37f2
to
08e96c4
Compare
08e96c4
to
113def4
Compare
113def4
to
eafa121
Compare
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.
@erwsmith - Thanks for working on this! I went ahead and did a brief review with some comments, but I have a main overall thought/question that I think will be most important for further iterations. Also, I'll leave a workflow for getting this down to one commit, which is all it currently needs as you're just updating the one file.
My overall question/thought is how to have both the emoji toy feature and the real mandatory topics feature information available to the user. Currently, I think one or the other should be included in either tip/help or a titled admonition blocks at strategic spots in the tutorial flow ...
Also, I think it'd be good to explicitly say somewhere (at the very beginning maybe) that one could do the example walk through feature or use this tutorial as a guide while implementing a new feature.
I like posting screenshots of the updated documentation in my pull requests for these types of updates (you can put them in HTML <details>
tags like I did above so that they don't take up a lot of screen area). Since the ReadtheDocs is generated as part of the CI, you could also add a comment with a link to the relevant parts of that has been updated, like this link to the example feature header
For the git commit flow, here's what I would do:
- On your local main branch, fetch the upstream branch (which should be the zulip/zulip repository main branch):
git fetch upstream
- Rebase your local main to the upstream:
git rebase upstream/main
- Rebase your local pull request branch to that:
git rebase upstream/main docs-new-feature-emoji-flag
. You shouldn't have any merge conflicts because you're the only one who is working on this documentation at the moment. - The previous command will have switched you to your local pull request branch, and you can now do an interactive rebase for your 3 commits:
git rebase -i HEAD~3
- In the interactive rebase, squash the 2nd and 3rd commits with the first and update the commit message for that one commit. The lint commit is not something that needs to be in the git history. I usually try to lint before committing or amending my commit after linting. And the updating of the example should be part of the main commit instead of a separate commit.
- Once you're down to the one commit, then you should be good. I would probably keep this to one commit and just do
git commit --amend
as you work through feedback and updates.
If you get confused during the interactive rebase, then do git rebase --abort
and things should go back to where you were before you started.
If you still have questions/issues, I'd post/read the #git help stream on CZO as there are folks there who are much more experienced in using git and github than I.
for the feature on the admin settings page, communicating and saving updates | ||
This example describes the process of adding a new setting to Zulip. | ||
The implementation of this setting is similar to that of | ||
an actual Zulip feature. You can review [the original commit for this feature](https://github.com/zulip/zulip/pull/5660/commits/aeeb81d3ff0e0cc201e891cec07e1d2cd0a2060d) |
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 link and sentence don't really make sense here now. I do think having references / notes to the mandatory topics checkbox could be helpful. Perhaps we can move them to some tip or hint blocks that follow along with the tutorial, and include some instructions for using git-grep
as well to help navigate the codebase.
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.
|
||
### Update the model | ||
|
||
First, update the database and model to store the new setting. Add a new | ||
boolean field, `mandatory_topics`, to the Realm model in | ||
boolean field, `emoji_flag`, to the Realm model in |
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.
My inclination would be to make this field name more clearly an example, like my_favorite_emoji
or something. I'm thinking something that would be easy to grep for as well.
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.
Sounds good, changed all to my_favorite_emoji
# ... | ||
+ mandatory_topics=bool, | ||
allow_message_editing=bool, | ||
+ emoji_flag=bool, |
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 section is definitely alphabetized now, so I'd put the example in maybe like this ...
property_types: Dict[str, Union[type, Tuple[type, ...]]] = dict(
allow_message_editing=bool,
# ...
+ my_favorite_emoji=bool,
# ...
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.
Great! Done.
// ... | ||
// Organization settings | ||
realm_allow_edit_history: $t({defaultMessage: "Enable message edit history"}), | ||
+ realm_emoji_flag: $t({defaultMessage: "Paste your favorite emoji here!"}), |
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.
From your screenshot, this text is a prompt for the emoji correct? I don't think this reads clearly in the rendered documentation. It looks like this is the text the reader should add.
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.
I went ahead with my suggested change above, and I added some more text to facilitate understanding of the end goal, but I'll be happy to edit more if it's not quite clear.
task of requiring messages to have a topic, you can [view this commit](https://github.com/zulip/zulip/commit/90e2f5053f5958b44ea9b2362cadcb076deaa975). | ||
|
||
You can view this feature rendered in the browser if you have the | ||
Zulip server running. It's the "Require topics in stream messages" |
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.
"Zulip server" is probably confusing here. I think we should specifically refer to (and maybe link to the documentation for) the development environment.
Also, let's bold the setting title instead of using quotes: "It's the Require topics in stream messages checkbox ..."
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.
would this be the correct wording: "...if you are in the development environment." ?
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.
or perhaps "...if you are in the Zulip development environment."
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 bolded the settting title and went ahead with "Zulip development environment." for now, but I'll be happy to change it if there's a better way to word it :)
e2f5757
to
9910048
Compare
f1592cf
to
851f746
Compare
This changes the new application feature tutorial to guide the developer through creating a new 'toy' feature. There is no issue associated with this.
b542f36
to
dc130ab
Compare
@laurynmm ready for re-review. |
Okay, so reading through the updated tutorial, a few things drew my attention about this overall change:
I think the next step (to make the current tutorial structure/text work with the example emoji feature instead of the mandatory topics feature) means making a deeper revision of the current guide to make sure that there is flow and consistency. But I would also consider that maybe the example emoji feature should be an additional section at the end of the tutorial, maybe as a challenge or follow-up task, that a person could choose to do or not. One argument for this is that folks tend to use the current example feature guide when implementing a real new feature (I've done this at least 3 times), so too much emphasis on a not real world example might take away from it's effectiveness in that regard. Unfortunately, we might not know which is the better option until doing a decent amount of work in one direction to see if it works. |
Thanks @laurynmm! I think your idea of splitting the two examples up completely is my favorite, considering that people have successfully used the original |
Heads up @erwsmith, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
Hello @zulip/server-emoji members, this pull request was labeled with the "area: emoji" label, so you may want to check it out! |
This changes the new application feature tutorial to guide the developer through creating a new 'toy' feature: a flag with a hardcoded emoji as the
defaultMessage
.Fixes: There is no issue associated with this.
Screenshots and screen captures:
Here's the outcome of the completed tutorial with an example emoji:
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: