-
Notifications
You must be signed in to change notification settings - Fork 528
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
Add ability to see and recycle sent messages #3353
Conversation
b579f21
to
3a3d3e6
Compare
Known bugs:
|
@mathjazz - testing from a user perspective everything seems nice and functional. Liking it! |
Makese sense, I'll play with it a little.
That's what the spec says. It's also what we've been using in the recent emails. But not in manual notifications. Maybe leave it for now and adapt if data shows it's not the right default?
This is checked implicitly, because the number of recipients in cases like this will be zero. Same as above, I'd add an explicit check if this actually turns out to be a problem. |
4450270
to
fe89f15
Compare
Made some minor adjustments to the overall styling, including this. |
fe89f15
to
a8242d0
Compare
a8242d0
to
bcbe6e4
Compare
…lace custom code
bcbe6e4
to
b9f5b0a
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.
Sorry for the time it took me to get to this. A few small things inline, but I'd appreciate a chance to see this on stage again and take it for a spin?
// Keep default middle-, control- and command-click behaviour (open in new tab) | ||
if (e.which === 2 || e.metaKey || e.ctrlKey) { | ||
return; | ||
} |
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.
How about shift+click?
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.
We'd need to file a bug for that and change it across all pages with tabs.
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.
The translate frontend already checks for shiftKey
when it checks for ctrlKey
, so we shouldn't deviate from that more in the places where we do special stuff on clicks.
// Keep default middle-, control- and command-click behaviour (open in new tab) | |
if (e.which === 2 || e.metaKey || e.ctrlKey) { | |
return; | |
} | |
// Keep default middle-, shift-, control- and command-click behaviour (open in new tab) | |
if (e.which === 2 || e.metaKey || e.ctrlKey || e.shiftKey) { | |
return; | |
} |
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'll update both files.
Deployed to stage. |
</div> | ||
<div class="footer"> | ||
<a href="{{ url('pontoon.messaging.edit_as_new', message.pk) }}" class="edit-as-new"><i class="fa fa-pencil-alt"></i>Edit as new</a> | ||
<time>{{ message.sent_at|format_for_inbox }}</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.
I initially didn't see this "sent at" info at all. I would expect for this to be near the top of the message, where we also have the sender, the recipient count & message type.
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.
It's a common pattern to show timestamp under the message bubble in the messaging apps. I think you'll get used to it quickly.
It'll be busy in the header.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files☔ View full report in Codecov by Sentry. |
Also included: * After the message is sent, show it in the Sent panel. * Remove now obsolete updating of custom checboxes (see f6ce601) * Fix comments in the Localizations URL patterns
… Send to myself, don't redirect to Sent.
58bb348
to
2abf95c
Compare
Updated and re-deployed to stage. I also had to rebase. |
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.
Looks good to go. Instead of rebasing, doing a merge from main
to the PR branch would make the changes easier to review.
Fix #3246: After the message is sent, it gets stored to the DB and displayed in the
Sent
panel.Fix #3247: Add ability to edit sent messages as new
Since @eemeli is on a PTO, I'll be integrating my work on #3247 directly into this PR. That will allow opening sent messages in the Compose panel, revealing more details about them, like the selected filters. It'll also make Sent and Compose panels accessible via URL, which will allow redirecting to the (updated) Sent panel after the message is sent (right now you need to refresh the page to see the sent message, which is annoying).
@bcolsson Could you please take this for a spin on stage? Note that temporarily messages are always sent to the sender, regardless if you click
Send to myself
orSend
. You can also manage messages via Django Admin.Also included are some under the hood changes that will make code more maintainable:
Updated on Sep 19, 14:20 UTC: I've now integrated the fix for #3247. Related to that: