Skip to content

Add more time-based variables for snippets #46049

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.

Sign up for GitHub

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

Jump to bottom
Merged
jrieken merged 3 commits into from
Mar 20, 2018
Merged

Add more time-based variables for snippets #46049

jrieken merged 3 commits into from
Mar 20, 2018

Conversation

@usernamehw
Copy link
Contributor

@usernamehw usernamehw commented Mar 18, 2018

Fixes #43140

Not 100% sure about short months: Sep vs Sept. They currently all 3-letter abbreviations and can be easily aligned:

17 Aug 2018
06 Jul 1999

Sep 20,2015
Jun 19,2000

If it's critical it can be changed to Sept. In that case Jun & Jul would be June & July, I guess...

If someone wants localized versions - New issue button awaits.

@jrieken jrieken added feature-request Request for new features or functionality snippets labels Mar 19, 2018
@jrieken
Copy link
Member

jrieken commented Mar 19, 2018

If someone wants localized versions - New issue button awaits.

We want it now. Please use vs/nls#localize to mark the strings are natural language string

@jrieken jrieken added this to the March 2018 milestone Mar 19, 2018
@usernamehw
Copy link
Contributor Author

usernamehw commented Mar 19, 2018

Are we talking about changing already defined ones or adding 4 new variables? There might be cases in which someone with localized version would want to show the default (English).

@jrieken
Copy link
Member

jrieken commented Mar 19, 2018 •
edited
Loading

Are we talking about changing already defined ones

Those defined in this PR, also don't substring the long variants but have them separately as short versions.

jrieken
jrieken requested changes Mar 20, 2018
View reviewed changes
src/vs/editor/contrib/snippet/snippetVariables.ts Outdated

resolve(variable: Variable): string {
const { name } = variable;
const dayNames = [nls.localize('Sunday', "Sunday"), nls.localize('Monday', "Monday"), nls.localize('Tuesday', "Tuesday"), nls.localize('Wednesday', "Wednesday"), nls.localize('Thursday', "Thursday"), nls.localize('Friday', "Friday"), nls.localize('Saturday', "Saturday")];
Copy link
Member

@jrieken jrieken Mar 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It would be better to keep those as static readonly propoerties because we don't need to recompute those string-arrays all the time. Otherwise looking good.

jrieken
jrieken reviewed Mar 20, 2018
View reviewed changes
Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

lgtm. thanks

@jrieken jrieken merged commit 04d155f into microsoft:master Mar 20, 2018
@usernamehw usernamehw deleted the additional_time_variables branch March 20, 2018 15:09
@jrieken jrieken mentioned this pull request Mar 26, 2018
Closed
2 tasks
@jrieken jrieken added the release-notes Release notes issues label Mar 28, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jrieken jrieken jrieken requested changes

Assignees

@jrieken jrieken

Labels

feature-request Request for new features or functionality on-testplan release-notes Release notes issues snippets

Projects

None yet

Milestone

March 2018

Development

Successfully merging this pull request may close these issues.

Additional date-based snippet variables

2 participants

@usernamehw @jrieken