Skip to content
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

Provide a way to add a timestamp to a comment #139524

Closed
alexr00 opened this issue Dec 20, 2021 · 24 comments
Closed

Provide a way to add a timestamp to a comment #139524

alexr00 opened this issue Dec 20, 2021 · 24 comments
Assignees
Labels
api api-finalization comments Comments Provider/Widget/Panel issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders ux User experience issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@alexr00
Copy link
Member

alexr00 commented Dec 20, 2021

Example of where this could be used: microsoft/vscode-pull-request-github#121

Some ideas for how this could look:

image

@alexr00 alexr00 added feature-request Request for new features or functionality comments Comments Provider/Widget/Panel issues labels Dec 20, 2021
@alexr00 alexr00 added this to the January 2022 milestone Dec 20, 2021
@alexr00 alexr00 self-assigned this Dec 20, 2021
@alexr00 alexr00 added api api-proposal ux User experience issues labels Dec 20, 2021
@alexr00
Copy link
Member Author

alexr00 commented Dec 21, 2021

FYI @laurentlb, I'm planning to start work on this soon.

@laurentlb
Copy link
Contributor

To get a UX consistent with GitHub, I think we'll need two fields:

  • one for display (e.g. 2 hours ago or on Aug, 1 2018)
  • one for hover (on GitHub, it's used for the full date with timezone). Other extensions could use it to show multiple timezones.

@hermannloose
Copy link
Contributor

hermannloose commented Dec 21, 2021

I just discussed another idea with @laurentlb:

  • have a single timestamp field on Comment objects (most likely a Date)
  • allow registering a formatter in CommentController, which when provided, is called with these timestamps and returns a string or optionally a more complex object, e.g. { label: string, tooltip?: string, ...} or {editorLabel: string, editorTooltip?: string, panelLabel: string, panelTooltip?: string}
  • provide reasonable default formatting if no formatter was registered

This seems the most flexible with regards to to tailoring an extension (e.g. "our company is in these three timezones, which should be available from the tooltip") as well as allowing a user to easily configure this (e.g. "I want this exact format string for long dates" or "I never want relative dates"), in the settings of an extension, or the settings of core VS Code itself (for the default formatting when no formatter was registered).

edit: For responsive relative dates (specifically "X mins ago"), the formatter would probably also need a way to influence when it is called again for the timestamp of a particular comment. Coming from RXJS, this seems like a fitting place to return an observable—all updates owned by the formatter—but this is not something I have encountered in VS Code code so far. An alternative with more manual house-keeping necessary on the VS Code side would be including another field in the object returned for a formatted date, which indicates when it will go stale. This could be an enum for more constraints from VS Code, e.g. "call me roughly every minute" vs "call me roughly every hour". Alternatively, just a relativeDate: boolean and determine this in VS Code based on the difference between the timestamp and now, at the (mostly theoretical) cost of not performing smoothly for "commented 90 minutes ago".

@alexr00
Copy link
Member Author

alexr00 commented Dec 29, 2021

Current proposal:

declare module 'vscode' {
export interface Comment {
timestamp?: {
date: Date,
label?: string,
useRelativeTime?: boolean
}
}
}

@alexr00
Copy link
Member Author

alexr00 commented Dec 29, 2021

Note from early discussions: We usually leave display decisions up to the users. Instead of having useRelativeTime be part of the API, we could have a setting. If extensions disagree they can always change the default value of the setting.

@alexr00
Copy link
Member Author

alexr00 commented Jan 11, 2022

For flexibility, we're now looking at using detail instead:

export interface Comment {
detail?: Date | string
}

To align with the timeline API proposal, should Date be number?

@alexr00
Copy link
Member Author

alexr00 commented Jan 12, 2022

After further discussing, we are keeping Date. The proposal is now in main:

export interface Comment {
/**
* An optional detail that will be displayed less prominently than the `author`.
* If a date is provided, then the date will be formatted according to the user's
* locale and settings.
*/
detail?: Date | string
}

Whether relative or precise time is used for display is controlled by the setting comments.useRelativeTime (name will likely change).

I have not tested other locales, but VS Code should handle displaying Dates correctly depending on locale. I will test this in the next couple days.

And finally, I didn't add anything to update the relative time yet. It will be updated every time the comment is rendered, so switching files will cause it to update. If this doesn't feel responsive enough then we can add some periodic updating.

@laurentlb and @hermannloose please let me know what you think of this proposal.

@laurentlb
Copy link
Contributor

That's great, thanks!
I like this proposal more than the previous one.

@alexr00
Copy link
Member Author

alexr00 commented Jan 12, 2022

I had to revert the change in main due to a loading issue. Will comment again when it's back.

alexr00 added a commit that referenced this issue Jan 13, 2022
@alexr00
Copy link
Member Author

alexr00 commented Jan 13, 2022

API proposal is back in main today.

@alexr00
Copy link
Member Author

alexr00 commented Jan 19, 2022

An idea of what this could look like in the comments panel:

image

Changes in that image include:

  • File icons
  • Timestamp
  • Long comment text gets truncated.

@isidorn
Copy link
Contributor

isidorn commented Jan 19, 2022

@alexr00 maybe allow a bit more characters before truncate.
It would be cool if each part would show different things on hover. So ideally hover on snippet would show the full comment.

@alexr00
Copy link
Member Author

alexr00 commented Jan 19, 2022

@isidorn that's exactly how it works :)

Recording 2022-01-19 at 16 13 48

I will make the length before truncate longer.

@albertelo
Copy link

Thanks for screenshot, and the progress on this issue!
I have a thought around the layout: the examples in the screenshot are a short, but what happens when the comments are longer? Would it make sense to keep the username and timestamp one row and the comment beneath it? I know this adds to vertical density, but you gain horizontal screen real estate for the comment and thus you can view more context before having to click on it. Having a full line for the username and timestamp also allows you to display the date and time without shortening the length of the comment.

@isidorn
Copy link
Contributor

isidorn commented Jan 19, 2022

@albertelo @alexr00 and me discussed this, and we thought that showing a comment "snippet" would be the best to make this view function as a good "bird's eye perspective" of what is going on. Problem with making it multi line is that it adds visual complexity - especially if there is a parent child comment relationship.
I definitely think we can iterate here, and would be willing to hear feedback from users on your side.

@albertelo
Copy link

At a high level what we found from our users was some trouble distinguishing the information that is shown. Username, time stamp and comment look similar and discerning that info might be hard. Adding a single node for the comment could help, and it would be done without having to add additional visual elements (e.g. username profile image or icons). That said, I recognize that if you have many nodes then the information density increases and so does the visual complexity.
Keeping in mind the "bird's eye perspective" idea, and going back to the first screenshot. What do you think on rather using only the time stamp and line code information to provide a "bird's eye perspective"? This reduces the visual complexity and provides context. Reading the comments would happen within the code, which is where it's likely going to happen even if we use short previews in the nodes.

@isidorn
Copy link
Contributor

isidorn commented Jan 20, 2022

@albertelo thanks for feedback. I agree that your proposed design in the first screen shot reduces visual complexity since it does not show comment snippets. However then the items would look too similar and it would be hard for the user to discern which item in the comments view to click on - especially in the case when all the comments are from the same team mate. This seems to align with what you heard from your users.

Can you maybe provide a sketch for how the single node comment idea would look? How would that work if there are replies which are already tree children?

alexr00 added a commit that referenced this issue Jan 21, 2022
@alexr00
Copy link
Member Author

alexr00 commented Jan 21, 2022

The proposal keeps getting smaller:

export interface Comment {
/**
* An optional timestamp that will be displayed in comments.
* The date will be formatted according to the user's locale and settings.
*/
timestamp?: Date;
}

Moved away from detail and now we're all in on calling it a timestamp that can only be a date. detail originally was intended to provide more freedom, but in reality this should only ever be a timestamp.

@laurentlb
Copy link
Contributor

laurentlb commented Jan 21, 2022

Thanks, the API proposal looks good!

we thought that showing a comment "snippet" would be the best to make this view function as a good "bird's eye perspective" of what is going on. Problem with making it multi line is that it adds visual complexity - especially if there is a parent child comment relationship.

I also like the comment snippet, it lets me find discussions quickly. Have you considered showing only the last comment of each thread? I think hiding the intermediate comments removes the visual complexity, and that would let us use two lines per thread (instead of one line per reply).

Quick draft to show the idea (3 distinct threads are visible):
image

The icon shows the status of each thread (e.g. resolved / unresolved). If needed, intermediate replies could be as child notes (but collapsed by default). The sketch is inspired from the Problems panel.

@isidorn
Copy link
Contributor

isidorn commented Jan 24, 2022

@laurentlb Thanks for the sketch. I personally like the idea of showing the snippet of the latest comment in the thread. Maybe we should have some simple ... affordance so user understands that there is more context before it.
As for showing the comment on multiple lines. I am not sure if this will work because the panel has more horizontal than vertical space. And I think we should be careful with introducing multi line elements.

However I think it is worth trying out both approaches and @alexr00 and me will consider these ideas and once we try them out we will know more. Thanks!

@alexr00 alexr00 changed the title Provide a way to add a timestamp or detail to a comment Provide a way to add a timestamp to a comment Feb 9, 2022
@alexr00
Copy link
Member Author

alexr00 commented Feb 9, 2022

Confirmed that OS language changes the exact time format, and that when the appropriate language extension is installed the relative time is in that language. Example in French:

image

image

@alexr00 alexr00 closed this as completed in a330b89 Feb 9, 2022
@isidorn
Copy link
Contributor

isidorn commented Feb 9, 2022

@laurentlb @albertelo please try it out with VS Code insiders from tomorrow and let us know what you think. We are looking forward to the feedback. Thanks!

@albertelo
Copy link

Tried it out today! Thanks!!
Looks good to me:
Screenshot 2022-02-10 at 17 54 00

@isidorn
Copy link
Contributor

isidorn commented Feb 11, 2022

Great - thanks for letting us know. Adding verified label.

@isidorn isidorn added the verified Verification succeeded label Feb 11, 2022
@Tyriar Tyriar added the verification-needed Verification of issue is requested label Feb 23, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-finalization comments Comments Provider/Widget/Panel issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders ux User experience issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants
@hermannloose @laurentlb @isidorn @Tyriar @albertelo @alexr00 and others