-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Closes #1040 Adds feature: Dropdown format date/time #2505
base: main
Are you sure you want to change the base?
Closes #1040 Adds feature: Dropdown format date/time #2505
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 this! This is a clever use of the existing token popup pattern.
I've added some comments and requested some changes.
Also not yet sure if we should change the popup behavior for the after you select a date choice -- should it then close (which would be different than the tokens). Need to play with it more.
Will also need to think about the other patterns supported since GL uses the https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat now (with backward compat to moment-style formats)
@@ -28,6 +28,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p | |||
- Ensures new worktrees are created from the "main" repo, if already in a worktree | |||
- Adds a new _remote_ command to the _Git Command Palette_ to add, prune, and remove remotes | |||
- Adds a _Create Worktree for Pull Request via GitLens..._ context menu command on pull requests in the _GitHub Pull Requests and Issues_ extension's views | |||
- Adds sugested options for date and time formats — closes [#1040](https://github.com/gitkraken/vscode-gitlens/issues/1040) |
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.
Typo: suggested
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.
@@ -278,7 +279,16 @@ export abstract class AppWithConfig<State extends AppStateWithConfig> extends Ap | |||
const $popup = document.getElementById(`${element.name}.popup`); | |||
if ($popup != null) { | |||
if ($popup.childElementCount === 0) { | |||
const $template = document.querySelector<HTMLTemplateElement>('#token-popup')?.content.cloneNode(true); | |||
let $template; | |||
if ( |
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.
Let's use an attribute or something here that avoids hard-coding the date input names
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.
done 4c7a5e2
|
||
const token = `\${${element.dataset.token}}`; | ||
let token!: string; | ||
if ( |
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.
Same here
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.
done 4c7a5e2
input.name == 'defaultTimeFormat' || | ||
input.name == 'defaultDateFormat' | ||
) { | ||
token = `${element.dataset.token}`; |
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.
Shouldn't need the template string here -- just:
token = `${element.dataset.token}`; | |
token = element.dataset.token; |
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.
done - 0c7c824
</div> | ||
<span class="token-popup__hint"> | ||
<i class="icon icon__info"></i> | ||
<a href="https://github.com/gitkraken/vscode-gitlens/wiki/Custom-Formatting" title="Open formatting docs" |
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 get a Wiki page for date formatting here
/> | ||
<label for="defaultDateFormat" title="See some sugested date/time formats"> |
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.
Typo: suggested
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.
done b69157a
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.
(Good day) Seems like this branch isn't up to date!
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.
Yup, that's why I started to work on my own branch with these commits
Edit
#2623
@@ -69,17 +69,23 @@ <h2>Dates & Times</h2> | |||
</span> | |||
</div> | |||
|
|||
<div class="setting"> | |||
<div class="setting__input"> | |||
<div class="setting" data-enablement="currentLine.enabled"> |
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.
Shouldn't have the data-enablement
set here.
<input | ||
id="defaultDateFormat" | ||
name="defaultDateFormat" | ||
type="text" | ||
placeholder="defaults to MMMM Do, YYYY h:mma" | ||
data-setting | ||
data-setting-preview | ||
data-popup-trigger | ||
disabled |
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.
Shouldn't have disabled
here
/> | ||
<label for="defaultDateShortFormat" title="See some sugested date/time formats"> |
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.
Typo: suggested
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.
done b69157a
/> | ||
<label for="defaultTimeFormat" title="See some sugested date/time formats"> |
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.
Typo: suggested
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.
done b69157a
@marcoapcaldas could I support you to close some of the comments on this PR? |
input.selectionEnd ?? selectionStart, | ||
)}`; | ||
if ( | ||
input.name == 'defaultDateShortFormat' || |
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 related with above commit - to https://github.com/gitkraken/vscode-gitlens/pull/2505/files/09bea8d0b5ddf478cc31262e9442e0d702185bd8?diff=split&w=0#r1137539072
done - 4c7a5e2
Description
This changes adds a dropdown menu with some suggestions for format date and time.
closes [#1040]
Checklist
Fixes $XXX -
orCloses #XXX -
prefix to auto-close the issue that your PR addresses