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

Add more time-based variables for snippets #46049

Merged
merged 3 commits into from Mar 20, 2018

Conversation

Projects
None yet
2 participants
@usernamehw
Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

jrieken commented Mar 19, 2018

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.

@@ -182,6 +187,10 @@ export class TimeBasedVariableResolver implements VariableResolver {

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")];

This comment has been minimized.

@jrieken

jrieken Mar 20, 2018

Member

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

jrieken left a comment

lgtm. thanks

@jrieken jrieken merged commit 04d155f into Microsoft:master Mar 20, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@usernamehw usernamehw deleted the usernamehw:additional_time_variables branch Mar 20, 2018

@jrieken jrieken added the on-testplan label Mar 26, 2018

@jrieken jrieken referenced this pull request Mar 26, 2018

Closed

Test new snippet variables #46556

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