Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Closes #1040 Adds feature: Dropdown format date/time #2505

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

Open
marcoapcaldas wants to merge 2 commits into gitkraken:main
base: main
Choose a base branch
Loading
from marcoapcaldas:feature/date-format-dropdown

Conversation

@marcoapcaldas
Copy link

@marcoapcaldas marcoapcaldas commented Feb 13, 2023

Description

This changes adds a dropdown menu with some suggestions for format date and time.

closes [#1040]

image

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

pelukron reacted with thumbs up emoji
Copy link
Member

@eamodio eamodio left a 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)

pelukron reacted with rocket emoji
- 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: suggested

pelukron reacted with thumbs up emoji
Copy link

@pelukron pelukron Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ($popup.childElementCount === 0) {
const $template = document.querySelector<HTMLTemplateElement>('#token-popup')?.content.cloneNode(true);
let $template;
if (
Copy link
Member

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

Copy link

@pelukron pelukron Apr 7, 2023

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 (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link

@pelukron pelukron Apr 7, 2023

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}`;
Copy link
Member

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:

Suggested change
token = `${element.dataset.token}`;
token = element.dataset.token;

pelukron reacted with thumbs up emoji
Copy link

@pelukron pelukron Apr 7, 2023

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"
Copy link
Member

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

data-popup-trigger
disabled
/>
<label for="defaultDateFormat" title="See some sugested date/time formats">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: suggested

pelukron reacted with thumbs up emoji
Copy link

@pelukron pelukron Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done b69157a

Copy link
Contributor

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!

Copy link

@pelukron pelukron Apr 16, 2023 •
edited
Loading

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


<div class="setting">
<div class="setting__input">
<div class="setting"data-enablement="currentLine.enabled">
Copy link
Member

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.

data-setting
data-setting-preview
data-popup-trigger
disabled
Copy link
Member

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

data-setting-preview
data-popup-trigger
/>
<label for="defaultDateShortFormat" title="See some sugested date/time formats">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: suggested

pelukron reacted with thumbs up emoji
Copy link

@pelukron pelukron Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done b69157a

data-setting-preview
data-popup-trigger
/>
<label for="defaultTimeFormat" title="See some sugested date/time formats">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: suggested

pelukron reacted with thumbs up emoji
Copy link

@pelukron pelukron Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done b69157a

Copy link

pelukron commented Apr 6, 2023

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)

@marcoapcaldas could I support you to close some of the comments on this PR?

input.selectionEnd ?? selectionStart,
)}`;
if (
input.name == 'defaultDateShortFormat' ||
Copy link

@pelukron pelukron Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eamodio eamodio eamodio requested changes

+2 more reviewers

@sadasant sadasant sadasant left review comments

@pelukron pelukron pelukron left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /