-
-
Notifications
You must be signed in to change notification settings - Fork 669
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
changelog: Document changes since v27.163. #4800
Conversation
43b0633
to
325043c
Compare
Thanks @chrisbobbe ! The most salient thing is that the "Highlights for users" needs to be a lot shorter. More precisely: the release notes we enter for the Play Store and App Store need to be a lot shorter than that; and this spot is where we do the work of drafting those up 🙂 See previous entries for examples. Those have sometimes bumped up against the limits imposed by the Play Store and App Store; I think 500 characters is the limit on at least one of them. Apart from the platform-imposed hard limits, people just aren't realistically going to read more than a small handful of items here. So the main thing I do in this section is to cut it down to just the few that users are most likely to actually feel are meaningful for them. I'll also make some specific comments inline. |
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.
Comments below on the "Highlights for users" section.
I'll take a look next at the developer-facing section but not quite as closely, because it's not user-facing and doesn't have tight length constraints.
docs/changelog.md
Outdated
* Added a server-compatibility notice for self-hosted orgs running Zulip | ||
Server versions before 2.0.0. (PR #4750) |
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 one definitely doesn't make it into this section -- most users will never see it, and for those that do, the banner is its own announcement 😉
And nobody will see this and think "ah, that's a great feature, I've been wanting something like that", or "oh that bug, so glad they fixed it." If someone's happy about this feature, it'll only be from rather abstractly reasoning through how it will help cause things to get updated.
docs/changelog.md
Outdated
* Unclear, little-used "create group PM" option in the compose box removed. | ||
(PR #4777) |
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 doesn't go in this section -- most users probably never noticed that icon, and nobody will be excited it's gone. (Some will be pleased it's gone, from a UI-connoisseur perspective; but from the direct user perspective, it's the removal of an annoyance one hadn't even quite put a finger on.)
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.
And comments on the developer-facing section. Thanks again!
docs/changelog.md
Outdated
* Resolved issues (latest to earliest): PR #4794, PR #4707, PR #4789, PR | ||
#4520, #4766, PR #4787, PR #4785, PR #4784, PR #4780, PR #4786, PR #4721, | ||
PR #4779, PR #4778, PR #4334, PR #4727, PR #4574, PR #4737, #4715, PR | ||
#4777, PR #4774, PR #4709, PR #4750, #4765, PR #4730, #4758, PR #4761, PR | ||
#4762, PR #4731, PR #4717, PR #4700, PR #4710, PR #4760, PR #4749, PR | ||
#4736, PR #4667, PR #4755, PR #4753, #4722, PR #4514, PR #4741, #4604, PR | ||
#4728, PR #4739, PR #4732, PR #3955, #3540, #4006, #4323, #4734, #4264, PR | ||
#4716, PR #4711, PR #4634, PR #4699, PR #4697, PR #4703, PR #4680, PR | ||
#4544, PR #4698, PR #4686, PR #4690, PR #4678, PR #4689, #3517, #3395 |
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 the way I've been compiling this "Resolved issues" list is that each item in it really is an issue, i.e. either a bug or a feature request; when I list a PR, it's because we didn't file a GitHub issue for it.
So for example, taking from the top of this list:
- accountsReducer [nfc]: Followups on handling restart events #4794 wouldn't be on that list
- accountsReducer: Use
restart
events to update server info when we can. #4707 would, as it implements a feature - Run
yarn upgrade
, after treating some deps individually. #4789 wouldn't - Flow-related work on the path to RN v0.64 upgrade, part 2. #4520 wouldn't
- Followups on open-links-where setting, #4679 #4766 wouldn't, even though it's a GitHub issue -- it was really a code review, reified as a GitHub issue just to help with task-tracking
- dependabot: Remove config file. #4787 wouldn't. Though there might be an item above saying we tried out Dependabot, and mentioning dependabot: Remove config file. #4787.
- build(deps-dev): bump eslint from 7.14.0 to 7.28.0 #4785, build(deps): bump react-redux from 7.2.1 to 7.2.4 #4784, build(deps): bump @sentry/react-native from 2.2.1 to 2.4.3 #4780, build(deps-dev): bump eslint-plugin-react from 7.21.5 to 7.24.0 #4786 wouldn't -- just automated upgrades
- flipper ios: Pin to some specific versions, to fix build with Xcode 12.5. #4721 would, as it fixes a build issue. Oh but it'd go closer to the end of the list, because it was merged May 6.
- deps: Remove prop-types from devDependencies. #4779, dependabot: Add config file. #4778, build(deps): bump ini from 1.3.5 to 1.3.7 #4334, build(deps): bump ua-parser-js from 0.7.21 to 0.7.28 #4727, build(deps): bump y18n from 3.2.1 to 3.2.2 #4574, build(deps): bump lodash from 4.17.20 to 4.17.21 #4737 wouldn't -- Dependabot upgrades, and the change to enable it
- Stream settings screen doesn't make sense for non-subscribed streams #4715 definitely would -- a buggy bit of UI
- (OK, that gives the idea.)
It wouldn't be unreasonable to have a list that includes all kinds of changes, even those that aren't issues. Possibly it'd be less work to compile that way -- though I think probably not, as it makes the list quite a bit longer.
Definitely quite reasonable to experiment with different ways of writing these notes.
If we do make this list broader, though, it should have a correspondingly different heading. "Resolved issues and merged PRs"?
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 the many muted-user PRs all get listed here? The umbrella issue, #4655, is quite near to being resolved but hasn't been yet.
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 think in that situation it'd be OK to say something like "most of #4655".
Done in my next revision 🙂
325043c
to
be2cd11
Compare
Thanks for the reviews! Revision pushed. Please see a question at #4800 (comment); I've left all the muted-users PRs in the "Resolved issues" list for this revision but it'd be easy to do something else. |
Thanks for the revision! Looks good modulo the bit about the muted-users PRs. |
be2cd11
to
6b990db
Compare
Thanks for the review! Revision pushed. |
Thanks! Looks good except for this stray heading at the top, new in this revision: +## 27.163 (2021-06-04)
+ I'll go ahead and merge after removing that. |
6b990db
to
f61f84d
Compare
Oops, thanks; I added this temporarily so that |
No description provided.