Francesco Lodolo [:flod] Reporter
	Description • 10 years ago

Please put some focus on strings when reviewing, not just code. The English text that landed doesn't make any sense

error-start-after-end-on-same-date=End date must come after start date on the same date

In fact, the attached spec in bug 977050 talks about "End TIME must come after start TIME on the same date". And, even like that, I still think that's a weak copy, way too hard to understand.

The first consequence is that you'll have to land the string with a different ID at this point, and adapt the code.

	Francesco Lodolo [:flod] Reporter
	Comment 1 • 10 years ago

Matej, any thought on this string?

"End time must come after start time on the same date"

It's displayed when you edit an event, and the result is an event on a single day, but with end time before start time.
https://bug977050.bugzilla.mozilla.org/attachment.cgi?id=8463767

	Matej Novak [:matej]
	Comment 2 • 10 years ago

I actually see two different strings in the spec:

"End date must come after start date"

"End time must come after start time on the same date"

It would be great if we could change them both for consistency. Here's what I would suggest:

"The event can not end before its start date"

"The event can not end before its start time"

(Setting a needinfo? for Stephany to confirm whether we're using "can not" or "cannot" in FxOS)

	Evan Tseng [:evanxd] Assignee
	Comment 4 • 10 years ago

Thanks for Francesco's reminder. :)

	Harly Hsu[:harly]
	Comment 5 • 10 years ago

Francesco and Matej,
Previously, our calendar only display "End date must come after start date" when user set end time or date prior to the start time or date. The reason I added the "End time must come after start time on the same date" is because I want to emphasis to the user that only the time is incorrect. How about we use "End date must come after start date" & "End time must come after start time", is that more clear?

	Francesco Lodolo [:flod] Reporter
	Comment 6 • 10 years ago

(In reply to Harly Hsu from comment #5)
> Francesco and Matej,
> Previously, our calendar only display "End date must come after start date"
> when user set end time or date prior to the start time or date. The reason I
> added the "End time must come after start time on the same date" is because
> I want to emphasis to the user that only the time is incorrect. How about we
> use "End date must come after start date" & "End time must come after start
> time", is that more clear?

Personally I find Matej's suggestions much easier to understand at first glance, and the meaning doesn't change. When I ready the current ones, it's like I'm reading an equation, it feels less natural.

	Matej Novak [:matej]
	Comment 7 • 10 years ago

(In reply to Francesco Lodolo [:flod] from comment #6)
> (In reply to Harly Hsu from comment #5)
> > Francesco and Matej,
> > Previously, our calendar only display "End date must come after start date"
> > when user set end time or date prior to the start time or date. The reason I
> > added the "End time must come after start time on the same date" is because
> > I want to emphasis to the user that only the time is incorrect. How about we
> > use "End date must come after start date" & "End time must come after start
> > time", is that more clear?
> 
> Personally I find Matej's suggestions much easier to understand at first
> glance, and the meaning doesn't change. When I ready the current ones, it's
> like I'm reading an equation, it feels less natural.

I agree. I think the reason the original is confusing is that it uses the word "after," but the thing that has to come after actually appears first in the sentence. You have to read it a couple of times to make sure you understand it correctly.

	Harly Hsu[:harly]
	Comment 8 • 10 years ago

(In reply to Matej Novak [:matej] from comment #7)
> (In reply to Francesco Lodolo [:flod] from comment #6)
> > (In reply to Harly Hsu from comment #5)
> > > Francesco and Matej,
> > > Previously, our calendar only display "End date must come after start date"
> > > when user set end time or date prior to the start time or date. The reason I
> > > added the "End time must come after start time on the same date" is because
> > > I want to emphasis to the user that only the time is incorrect. How about we
> > > use "End date must come after start date" & "End time must come after start
> > > time", is that more clear?
> > 
> > Personally I find Matej's suggestions much easier to understand at first
> > glance, and the meaning doesn't change. When I ready the current ones, it's
> > like I'm reading an equation, it feels less natural.
> 
> I agree. I think the reason the original is confusing is that it uses the
> word "after," but the thing that has to come after actually appears first in
> the sentence. You have to read it a couple of times to make sure you
> understand it correctly.

OK, what you guys said make sense to me. So should we just go ahead with the strings below which you've suggested?
"The event can not end before its start date"
"The event can not end before its start time"
Thanks

	Francesco Lodolo [:flod] Reporter
	Comment 9 • 10 years ago

There's a pending NI for Stephany. Besides that, it would be nice to have this landing before FL (Sep 1).

I took a quick look at the strings, and it doesn't look we're very consistent with "can not" vs "cannot".
http://transvision.mozfr.org/?recherche=can+not&repo=gaia&sourcelocale=en-US&locale=en-US&search_type=strings

	Matej Novak [:matej]
	Comment 10 • 10 years ago

(In reply to Francesco Lodolo [:flod] from comment #9)
> There's a pending NI for Stephany. Besides that, it would be nice to have
> this landing before FL (Sep 1).
> 
> I took a quick look at the strings, and it doesn't look we're very
> consistent with "can not" vs "cannot".
> http://transvision.mozfr.org/?recherche=can+not&repo=gaia&sourcelocale=en-
> US&locale=en-US&search_type=strings

Given that we're so inconsistent already, I think we can move forward with this now and then clean up all those strings as part of the copy audit that Stephany is doing.

	Stephany Wilkes
	Comment 11 • 10 years ago

The copy audit started late and will not be done for 2.1. Matej's suggestions should be included and there is plenty of time for them to land ahead of string freeze on 9/2.

	Stephany Wilkes
	Comment 12 • 10 years ago

Also, "cannot" is correct, but we will use contractions going forward, so "can't" would be acceptable as well. Thanks!

	Matej Novak [:matej]
	Comment 13 • 10 years ago

(In reply to Stephany Wilkes from comment #12)
> Also, "cannot" is correct, but we will use contractions going forward, so
> "can't" would be acceptable as well. Thanks!

Thanks!

	Francesco Lodolo [:flod] Reporter
	Comment 14 • 10 years ago

(In reply to Stephany Wilkes from comment #11)
> The copy audit started late and will not be done for 2.1. Matej's
> suggestions should be included and there is plenty of time for them to land
> ahead of string freeze on 9/2.

Friendly reminder (and now going back offline).

	Evan Tseng [:evanxd] Assignee
	Comment 15 • 10 years ago

Comment on attachment 8477186 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23173

Hi Francesco,

Could you help to review the L10n labels[1]?
And thanks for the reminder.

[1] https://github.com/mozilla-b2g/gaia/pull/23173/files#diff-97b7fed57927556b91fb72d4d1b8c8b5R232

Thanks.

	Evan Tseng [:evanxd] Assignee
	Comment 16 • 10 years ago

Comment on attachment 8477186 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23173

Hi Matej,

Could you help to review the l10n label[1]?
Francesco is not available for this.
Thanks.

[1] https://github.com/mozilla-b2g/gaia/pull/23173/files#diff-97b7fed57927556b91fb72d4d1b8c8b5R232

	Matej Novak [:matej]
	Comment 18 • 10 years ago

(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #16)
> Comment on attachment 8477186 [details]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23173
> 
> Hi Matej,
> 
> Could you help to review the l10n label[1]?
> Francesco is not available for this.
> Thanks.
> 
> [1]
> https://github.com/mozilla-b2g/gaia/pull/23173/files#diff-
> 97b7fed57927556b91fb72d4d1b8c8b5R232

I don't think I'm the right person for this kind of review. I'm not sure what an "l10n label" is.

	Axel Hecht [:Pike]
	Comment 19 • 10 years ago

Comment on attachment 8477186 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23173

from irc, strings look good to Matej, IDs look good to me, f+.

	Evan Tseng [:evanxd] Assignee
	Comment 20 • 10 years ago

[Blocking Requested - why for this release]:

	Evan Tseng [:evanxd] Assignee
	Comment 21 • 10 years ago

This is a bug for a 2.1 feature(Bug 977050).

	Dylan Oliver [:doliver]
	Comment 22 • 10 years ago

triage: 2.1+ 

Improved strings required for a new feature in 2.1.

	Evan Tseng [:evanxd] Assignee
	Comment 23 • 10 years ago

Comment on attachment 8477186 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23173

Hi Miller,

Could you help to review the patch?

Thanks.

	Miller Medeiros [:millermedeiros]
	Comment 24 • 10 years ago

Comment on attachment 8477186 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23173

LGTM. minimal changes, and since it affected the string also updated the l10n IDs.

	Evan Tseng [:evanxd] Assignee
	Comment 25 • 10 years ago

Thanks, Miller.

	Evan Tseng [:evanxd] Assignee
	Comment 26 • 10 years ago

master: https://github.com/mozilla-b2g/gaia/commit/4f79670249b9638b9f7242116f6ffeb665ac545a

And the failure of Gij is not related with this patch. We already track the bug in Bug 1064305.

	Evan Tseng [:evanxd] Assignee
	Comment 27 • 10 years ago

Comment on attachment 8477186 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23173

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Bug 977050
[User impact] if declined: Show the wrong error message when user set end time before start time.
[Testing completed]: It works in local, and we have unit and integration tests for this.
[Risk to taking this patch] (and alternatives if risky): Low risk, we only have 10 lines change(not included test code).
[String changes made]: Yes, we changed two l10n strings[1].

[1] https://github.com/mozilla-b2g/gaia/commit/4f79670249b9638b9f7242116f6ffeb665ac545a#diff-97b7fed57927556b91fb72d4d1b8c8b5R232

	Evan Tseng [:evanxd] Assignee
	Comment 28 • 10 years ago

Thanks, Bhavana.

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 10 years ago

v2.1: https://github.com/mozilla-b2g/gaia/commit/8edb200d29ca5be9ba7f8bd200b8f4adbc5f59df

	Edward Chen[:edchen]
	Comment 30 • 10 years ago

[Environment]
Gaia      944e5b4582c4efa1b67cd33245dbb8f6aa25d09f
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/7546fedad918
BuildID   20140914160203
Version   34.0a2
ro.build.date   Fri Jun 27 15:57:58 CST 2014
ro.bootloader   L1TC00011230
ro.build.version.incremental   110


[Result]
PASS

	Yeojin Chung [:YeojinC]
	Comment 31 • 10 years ago

This issue is verified on Flame 2.2 and 2.1.

Result: "The event cannot end before its start date" and "The event cannot end before its start time" strings are displayed properly.

Flame 2.2

Device: Flame Master (319mb)(Kitkat Base)(Full Flash)
Build ID: 20141028040202
Gaia: 6a7fb482a03c5083ef79b41e7b0dfab27527cd04
Gecko: a255a234946e
Version: 36.0a1 (Master)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
  
Flame 2.1 

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141028001203
Gaia: a0174f7166745256aaca1cb3aa9f894033fbffa6
Gecko: 43bda3541f6b
Gonk: 6e51d9216901d39d192d9e6dd86a5e15b0641a89
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0