Skip to content
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

Standardise capitalisation throughout UI #5588

Merged
merged 1 commit into from Feb 1, 2015

Conversation

@goobertron
Copy link

commented Jan 25, 2015

This PR follows the vote in https://www.loomio.org/d/kY0wClao to use sentence case throughout the UI for the English locale, and fixes #5520. I have also made a few minor changes to the English, and changed special characters to “, &nash;, etc.

Have amended the two locale files affected (building on the work in #5579) and have amended the cuke tests which were broken by the changes to capitalisation.

For now I have just changed the text strings in the tests, as using the I18n.t('string') syntax doesn't work in the features themselves, only in the step-definitions. I don't want to hold up this PR while I work out how to make tests text-independent, so I'll do that in a separate PR.

I wanted to get this work finished today, but would like to wait a couple more days just in case serious objections arise to using sentence case for titles of parts of the app ('My activity', 'My aspects', etc.). They shouldn't, given the vote, but it's just possible that some voters didn't realise that these titles would be included, so I'd like to double-check that before proceeding.

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 25, 2015

The tests I have changed are passing locally, but some others, seemingly unrelated, are failing; mainly giving the error:

Modal dialog present: "This page is asking you to confirm that you want to leave - data you have entered may not be saved." (Selenium::WebDriver::Error::UnhandledAlertError)

I'm hoping this is either random or something to do with my Selenium set-up, and so won't affect Travis. Fingers crossed!

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 25, 2015

The modal dialog one is a random failure we have left, yes.

Why did you change quotes to HTML entities? Such escaping should be done while the translation is inserted into the view, was that not happening somewhere?

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 25, 2015

The escaping was happening; I just thought it would be safer to use the HTML entities. Not a good idea? I can easily change them back.

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 25, 2015

Yes, please change them back, having them in the translations assumes that they are only used in an HTML context, which might be true for the moment, but isn't very future proof ;)

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 25, 2015

That's a good point. Oh, the irony! Have reverted. Thanks for the quick comments. :)

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 25, 2015

I've just remembered: one reason I was led to change special chars to entities was that there were already existing » and « on lines 1363 & 1364. Want them changed also? And if so, should I use left/right angle-quotes or a pair of greater-than/less-than symbols?

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 25, 2015

Don't know from the top of my head, have to check where and how they are used when I find the time to. If you change them back, use the typographically correct symbols.

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 25, 2015

They appear under:

  will_paginate:
    previous_label: "« previous"
    next_label: "next »"

$ grep -Rin "will_paginate" app produces results:

app/views/contacts/index.mobile.haml:17:      = will_paginate @contacts, :renderer => WillPaginate::ActionView::BootstrapLinkRenderer
app/views/conversations/index.haml:24:            = will_paginate @conversations, :renderer => WillPaginate::ActionView::BootstrapLinkRenderer
app/views/conversations/index.mobile.haml:30:  = will_paginate @conversations, :renderer => WillPaginate::ActionView::BootstrapLinkRenderer
app/views/notifications/index.html.haml:55:      = will_paginate @notifications, :renderer => WillPaginate::ActionView::BootstrapLinkRenderer
app/views/notifications/index.mobile.haml:24:  = will_paginate @notifications, :renderer => WillPaginate::ActionView::BootstrapLinkRenderer
app/views/people/_index.html.haml:13:  = will_paginate people, :renderer => WillPaginate::ActionView::BootstrapLinkRenderer
app/views/people/contacts.haml:24:        = will_paginate @contacts_of_contact, :renderer => WillPaginate::ActionView::BootstrapLinkRenderer
app/views/people/index.html.haml:37:          = will_paginate(@people)
app/views/people/index.mobile.haml:30:    = will_paginate @people, :renderer => WillPaginate::ActionView::BootstrapLinkRenderer

I plan to leave them for this PR, and perhaps fix in a subsequent clean-up of that file. So if you want me to change them in this PR, just let me know, otherwise I'll leave them be.

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 25, 2015

Leave them, I'm not sure if will_paginate would escape them, probably not and that's why they're there that way in the first place.

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 25, 2015

Whoops, broke a few more tests which got lost among the random failure, will fix now.

@goobertron goobertron force-pushed the goobertron:text_edits branch 2 times, most recently from d712d87 to 62b7bd2 Jan 25, 2015

@ghost

This comment has been minimized.

Copy link

commented Jan 26, 2015

I notice it was missed in the original PR so worth a mention, this should also cover the items listed under 'Receive email notifications when:' as they are currently inconsistent with the other listed items on that /user/edit page too.

eg.

someone sent a report
someone starts sharing with you 

etc
@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 26, 2015

The original PR did capitalise those strings, but I believe they should not be capitalised, because that are items in a list following a colon, and are therefore not complete sentences in themselves. They originally had ellipses preceding each one, which I removed when I did some reordering and rewording of the settings page last year.

The vote doesn't mean 'stick a capital at the start of every string in en.yml' ;)

@ghost

This comment has been minimized.

Copy link

commented Jan 26, 2015

This brings us back to the whole point of the vote - which was to sort out the almighty inconsistency.

I'm not fussed either way if those items start with a capital or not, but if they don't, then neither should any other item on that page which appears under a bold heading.

So where we have for example:

Stream preferences

Show Community Spotlight in stream

Show Getting Started hints

(the same applies to 'Sharing settings', etc, the above is just an example)

We need to either:

  1. Put a colon at the end of "Stream preferences" and then start each item under that heading with a lower case letter

or

  1. Remove the colon from 'Receive email notifications when' and uppercase the first letter of each item in the list

I'm honestly not fussed either way, so long as it's consistent :)

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 26, 2015

The vote was consistently to use sentence case throughout the UI.

Show ‘community spotlight’ in stream
Show ‘getting started’ hints

are both complete sentences. 'Stream preferences' is a section heading; it is not part of the text below it. While many of

someone sends a report
someone starts sharing with you

are valid sentences, they are not complete sentences in the context of the page on which they appear. That is, their role on this page only make sense in the context of the text before the colon: the relevant action is in the text above the list; you're indicating, for example, not that you want someone to send a report but that you want to receive an email notification when this happens. Therefore they are not presented here as sentences and so should not have an initial capital.

I used to be a professional editor, so I'm pretty familiar with this stuff.

@ghost

This comment has been minimized.

Copy link

commented Jan 26, 2015

Ok, so let's change that heading then so that the email options part of that page is also a "section heading" too. No point in having just one area on that page that's not a section heading when the rest of them are. Consistency eh :)

How about changing it from 'Receive email notifications when:' to simply 'Email notifications' ?

Then adjust the listed items below that?

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 26, 2015

The equivalent section heading/integral items would be:

Email notification settings

• Receive email notifications when someone starts sharing with you
• Receive email notifications when you are mentioned in a post
• Receive email notifications when someone likes your post
• Receive email notifications when someone reshares your post
• Receive email notifications when someone comments on your post
• Receive email notifications when someone comments on a post you've commented on
• Receive email notifications when you receive a private message
• Receive email notifications when someone sends a report

Surely you must agree that this is unwieldy.

For consistency of heading presentation, we could certainly have

Email notification settings

Receive email notifications when:
• someone starts sharing with you
• you are mentioned in a post
• someone likes your post
• someone reshares your post
• someone comments on your post
• someone comments on a post you've commented on
• you receive a private message
• someone sends a report

  • but that's not related to capitalisation.

To repeat myself, the vote was consistently to use sentence case, not consistently to stick a capital at the start of every string in en.yml. Making the first letter of each of those list items would be an inconsistent use of sentence case.

@ghost

This comment has been minimized.

Copy link

commented Jan 26, 2015

Yes, I agree, your example there is completely unwieldy :)

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 26, 2015

I believe the remaining Travis failures are unrelated to this PR and hopefully just randoms.

click_here: "click here"
a_private_message: "There’s a new private message in diaspora* for you to check out."
a_limited_post_comment: "There’s a new comment on a limited post in diaspora* for you to check out."
email_sent_by_diaspora: "This email was sent by %{pod_name}. If you’d like to stop getting emails like this,"

This comment has been minimized.

Copy link
@jhass

jhass Jan 26, 2015

Member

Okay, weird issue.

The is apparently triggering some additional escaping in the email output, which makes a spec fail. I guess the encoding is right and the spec should probably be updated, but since I only guess that let's work around it for now and use the HTML-Entity for this case (’).

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 26, 2015

Ah, that explains it (and is weird). I can use the HTML entity or, which might perhaps make things simpler, just change them all back to basic upright apostrophes ( ' ). This is how they were presented before, but I changed them because (ironically, as it turns out) I thought it would be more robust this way as ' can also be used to denote starts and ends of strings in the file, so I thought removing them from text would be safer.

I'm happy to do whichever you think better for now.

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 28, 2015

Have pushed a commit replacing closing quotes with HTML entities. Let me know if you'd rather I did it a different way.

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 28, 2015

and use the HTML-Entity for this case

I should've highlighted the this case, sorry :/ I really only meant that single one, for the others my other comment still applies.

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 29, 2015

Ah, I thought 'this' referred to the case of data being pulled from a locale file. Sorry. Will amend.

@goobertron goobertron force-pushed the goobertron:text_edits branch 2 times, most recently from 90e5f10 to 7c61d01 Jan 29, 2015

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 29, 2015

Have used a standard upright apostrophe for that one case, as we know that works (it's what was used before this PR). Also added Changelog entry.

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 30, 2015

OK, I haven't managed to make Travis go green, but I'm confident the one remaining failure is unrelated (possibly because the text in the button concerned changed to 'Stop following #boss' on hover?).

@jhass

This comment has been minimized.

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 30, 2015

Oh, hell's biscuits, that's opened a whole new can of worms.

I can update that one no problem, but for instance I see in https://github.com/diaspora/diaspora/blob/develop/spec/javascripts/app/views/help_view_spec.js#L7 dozens of strings which might need updating - in fact I know that many of those have been altered by capitalisation or changes from upright single quotes to curly ones.

Am I going to need to go through all the spec files to check this? Because if so I think I'm going to have to drop this PR and let someone else pick it up who can face doing all that checking.

Is there any way of making those spec text-independent as I suggested above? E.g. using the I18n.t("string_ID") syntax you suggested? Something like

Diaspora.I18n.reset({
  'aspect_dropdown': {
  'select_aspects': I18n.t("javascripts.aspect_dropdown.select_aspects"),
  'all_aspects': I18n.t("javascripts.aspect_dropdown.all_aspects"),

If so it might be better to do that conversion work throughout /features and /spec before trying to get this PR merged. If it'll be straightforward I don't mind attempting it to make the tests more stable for the future.

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 30, 2015

It works for feature/ and spec/, it doesn't work for spec/javascripts.

Travis suggests that this is the only failure, so I'd just hardcode that instance, really. Sometimes making it work is more important than doing it right, especially if the effort of the later is 1000x (< 1 minute vs. several hours) higher ;)

@goobertron goobertron force-pushed the goobertron:text_edits branch from 7c61d01 to 5aae985 Jan 30, 2015

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 30, 2015

Sometimes making it work is more important than doing it right, especially if the effort of the later is 1000x (< 1 minute vs. several hours) higher ;)

Sure, and I'm happy to do that here. I'm just thinking of putting in some work now to make the tests more stable for the future, so some other poor sap doesn't find a load of broken tests from changing a few simple text strings...

Anyway, that test fixed for now. Fingers crossed!

goobertron
Amend all text strings in en locales to use sentence case
Adapt cukes to work with new capitalisation rules

@goobertron goobertron force-pushed the goobertron:text_edits branch from 5aae985 to 42cd233 Jan 31, 2015

@goobertron

This comment has been minimized.

Copy link
Author

commented Jan 31, 2015

OK, it turns out that failure in follows_tags.feature wasn't random after all. Sorry, Travis.

The problem was that the text in that test step should have been the mouse-over text, which is "Stop following #boss"; so, while the capitalisation of the displayed button text hadn't changed, that of the mouse-over text had. So, while the test step was faulty, it worked previously as the capitalisation of 'Following' was the same in both versions. Now fixed, and passing locally, so hopefully this time we'll get the green light!

@goobertron

This comment has been minimized.

Copy link
Author

commented Feb 1, 2015

Green at last! What a relief.

This has to be one of my worst PRs ever :(

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 1, 2015

Great! So do you still want to let it sit for a while or you'd say it's ready? :)

@jhass jhass added this to the next-major milestone Feb 1, 2015

@goobertron

This comment has been minimized.

Copy link
Author

commented Feb 1, 2015

There was no overwhelming request to keep My Aspects, My Activity and so on in title case, so I'm happy for this to be merged now.

jhass added a commit that referenced this pull request Feb 1, 2015

Merge pull request #5588 from goobertron/text_edits
Standardise capitalisation throughout UI

@jhass jhass merged commit 3747fd7 into diaspora:develop Feb 1, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jhass

This comment has been minimized.

Copy link
Member

commented Feb 1, 2015

Thank you very much!

@goobertron

This comment has been minimized.

Copy link
Author

commented Feb 1, 2015

You're welcome. And thanks for all your help getting this one through. I'll now look at making the tests text-independent and see if I can get anywhere useful with that.

@ghost

This comment has been minimized.

Copy link

commented Feb 1, 2015

@goobertron Props for sorting this one out man!! Much appreciated 👍 :)

@goobertron goobertron deleted the goobertron:text_edits branch Jun 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.