-
-
Notifications
You must be signed in to change notification settings - Fork 830
Make chat exports use ISO 8601/RFC 3339 dates and be more informative #8558
Conversation
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 for the PR! While I don't think the Product team cares about the date format as much, they might have thoughts on the file naming otherwise (such as the room name being awkward to look at when made filesafe, or the lack of brand
variable to denote where the file came from).
Requesting review from the product team to determine if this assumption is true.
src/utils/exportUtils/JSONExport.ts
Outdated
@@ -108,7 +108,7 @@ export default class JSONExporter extends Exporter { | |||
this.addFile("export.json", new Blob([text])); | |||
await this.downloadZIP(); | |||
} else { | |||
const fileName = `matrix-export-${formatFullDateNoDay(new Date())}.json`; | |||
const fileName = `${this.room.name}-${formatFullDateNoDayISO(new Date())}.json`; |
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.
applies throughout: what if the room name contains characters that can't be translated into a file name?
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.
ohh i understand, i probably should check the room name first, if it contains characters that can't be used as a filename, then i default to the brand name or so, how about that?
In what cases is "Element - Chat Export - […]" currently being used? Looking at the before screenshot |
When a chat is exported as an HTML |
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 the filename should be "Element-<roomname>-<ISOdate>
" in all cases. For example, Element-testroom-2022-05-12T12_00_00.735Z.json
I will leave it to the technical review on what exactly should happen with room names that can't be translated to filenames, but please feel welcome to ask for my input if needed.
Ok thanks, so should i go ahead and implement your suggestion? also concerning the room names that can't be translated to filenames, i already made a suggestion up there like we can check if the room name has some characters that's not acceptable in a filename with regex or so, if it does, we just ignore it and default to the brand name like |
@yaya-usman Yes, please implement. To confirm, filename should have the following order of preference:
|
@kittykat I have made adjustments based on your suggestions. Below images shows the result tested room names The following is the resulting filename It works for all the exports |
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.
LGTM.
One improvement could be to drop any leading and trailing whitespace in room name when the filename is put together. E.g. Room / /
could give a filename of Element-Room-2022-05-18T10_28_10.032Z.zip
instead of ``Element-Room -2022-05-18T10_28_10.032Z.zip`.
This means if the room name is say |
Mid-string spaces would stay where they are so ideally:
If this is not easy, then we can keep it as:
|
This PR contends with #7992 |
@yaya-usman Sorry to follow up again but because of the conflicting PR, can you please split out changing date format into its own PR and tag me in it for product signoff? We can merge that one as soon as possible, and then figure out how to handle the naming conflicts separately 👍 Thank you! |
Sure no problem, Sorry for the late response. I will be back fully when I am done with exams in school next weekend. |
@yaya-usman could you please split out this PR into two? We would like to merge the update to the date format on top of #7992 and then review the other changes to naming separately. |
Sorry for the late response, i thought this PR has been resolved or rather has been fixed by another person, i deleted the branch from my local but i guess i can still get it back. Also how do i split the PR, i don't really understand that part |
Here's a guide you could have a look at for splitting a PR and to split a commit, I would use an interactive rebase, edit the commit, then do an interactive add |
Thanks for the PR! After asking for things to be split up we talked about it more as a group and came to a slightly different conclusion on the file naming. That conclusion is now documented as #9440 (which includes your other PR in it). Hopefully this means the code lands sooner - apologies for the delay here. |
Fixes: vector-im/element-web#21812
it also includes the room name in the file export name.
Signed-off-by: Yaya Usman [email protected]
Before:
After:
Here's what your changelog entry will look like:
✨ Features