-
Notifications
You must be signed in to change notification settings - Fork 384
compose: Add feature to insert global time #2056
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
base: main
Are you sure you want to change the base?
Conversation
|
As usual, please include before/after screenshots demonstrating all your changes, in addition to any videos. |
e697f42 to
7b2490a
Compare
|
Thank you, I’ve added the after screenshots as well. |
chrisbobbe
left a comment
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.
Thanks @yash-agarwa-l! Comments below, from a brief review. I'll look more closely at the logic in a later review.
lib/widgets/compose_box.dart
Outdated
|
|
||
|
|
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.
nit: don't add extra lines
lib/widgets/compose_box.dart
Outdated
| _insertTextAtCursor(insert); | ||
| } | ||
|
|
||
| void _insertTextAtCursor(String insertion) { |
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.
Hmm, I'd thought that ComposeContentController already had a method for inserting inline content, using ComposeContentController.insertionIndex, but it looks like it doesn't. ComposeContentController.insertPadded is what we use for quote-and-reply (a kind of block content, i.e. not inline content):
/// Inserts [newText] in [text], setting off with an empty line before and after.
///
/// Assumes [newText] is not empty and consists entirely of complete lines
/// (each line ends with a newline).
///
/// Inserts at [insertionIndex]. If that's zero, no empty line is added before.
///
/// If there is already an empty line before or after, does not add another.
void insertPadded(String newText) {I propose adding a method insertInline, just above insertPadded, and renaming insertPadded to insertBlock so it makes more sense alongside the new method. Here's a draft of a dartdoc plus asserts; would you fill in the implementation?
/// Inserts [newText] in [text],
/// setting off with a space character before and after.
///
/// Assumes [newText] does not start or end with whitespace
/// (i.e. that [String.trim] would not return a different string).
///
/// Inserts at [insertionIndex]. If that's zero, no space is added before.
///
/// If there is already an empty space before or after, does not add another.
void insertInline(String newText) {
assert(newText.isNotEmpty);
assert(newText.trim() == newText);Then the global-time button would call insertInline.
lib/widgets/compose_box.dart
Outdated
| String formatGlobalTime(DateTime date) { | ||
| final iso = date.toIso8601String(); | ||
| final trimmedIso = iso.contains('.') ? iso.substring(0, iso.indexOf('.')) : iso; | ||
| final offset = date.timeZoneOffset; | ||
| final sign = offset.isNegative ? '-' : '+'; | ||
| final hours = offset.inHours.abs().toString().padLeft(2, '0'); | ||
| final minutes = (offset.inMinutes.abs() % 60).toString().padLeft(2, '0'); | ||
|
|
||
| return '$trimmedIso$sign$hours:$minutes'; | ||
| } |
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 is the kind of logic that belongs in lib/model/compose.dart; see other functions there. Maybe name this one just globalTime, and it can go at the end of the file.
| "@composeBoxAttachFromCameraTooltip": { | ||
| "description": "Tooltip for compose box icon to attach an image from the camera to the message." | ||
| }, | ||
| "composeBoxAttachGlobalTimeTooltip": "Insert global time", |
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.
Please add these entries in the same commit that makes them necessary.
| }); | ||
| }); | ||
|
|
||
| group('global time button', () { |
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.
Please add these tests in the same commit that makes them necessary.
| /// A button to insert a global time timestamp into the compose box. | ||
| /// | ||
| /// Shows date and time pickers sequentially, then inserts a formatted | ||
| /// timestamp in Zulip's global time format: `<time:YYYY-MM-DDTHH:mm:ss±HH:mm>` |
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.
Please add these dartdocs in the same commit that makes them necessary.
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.
Thanks! Just for clarity, you would like me to squash everything into the one commit that introduces the implementation logic, correct?
|
Are we controlling the date/time picker styling at all? The "Select date/time" text should perhaps be sized like a heading. |
7b2490a to
87bba96
Compare
Renames `insertPadded` to `insertBlock` for block content (line-padded), makingthe distinction between the two insertion types explicit. Fixes the issue described here: zulip#2056 (comment)
|
Thanks for the review. I’ve addressed the feedback and squashed the commits as requested. |
|
@alya Ack. It's using the default styling right now, but I can bump the size up to a heading. Is there a specific style you want there? |
Fixes zulip#1215. This adds a clock button to the compose box that opens date and time pickers sequentially. When completed, it inserts a timestamp in Zulip's global time format (e.g., <time:2025-12-31T13:30:00+05:30>). This allows users to easily reference specific times that display correctly across all timezones.
Renames `insertPadded` to `insertBlock` for block content (line-padded), makingthe distinction between the two insertion types explicit. Fixes the issue described here: zulip#2056 (comment)
f50ac53 to
7947f1c
Compare
Renames `insertPadded` to `insertBlock` for block content (line-padded), makingthe distinction between the two insertion types explicit. Fixes the issue described here: zulip#2056 (comment)
7947f1c to
6182186
Compare
|
I dunno, you can try our modal heading size and update the screenshots for review. |
Fixes #1215.
compose_date.mp4