Limit conversations (no Rooms) subject to 124 characters.

VERIFIED FIXED

Status

Hello (Loop)
Client
P3
minor
Rank:
33
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: standard8, Assigned: frsela)

Tracking

unspecified
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [chat bug])

Attachments

(2 attachments, 2 obsolete attachments)

Bug 1107082 implemented a limit on the number of characters for a room name. I think we should probably somehow reflect that in the UI - possibly one of:

- find out server limit
- hard-code the limit
- Allow the user to do any length, detect the server failure and reflect it in an informed way to the user.
Sevaan: We should probably be doing client-side limiting of this even though the server limit added in bug 1107082 doesn't apply now.

Is 124 characters a reasonable number? (this was originally as suggested by the FxOS client, but I have no details as to why it was that number).
Flags: needinfo?(sfranks)
(In reply to Mark Banner (:standard8) from comment #1)

> Is 124 characters a reasonable number? (this was originally as suggested by
> the FxOS client, but I have no details as to why it was that number).

No reasons from our FxOS client (at least from a UX point of view), Alexis suggested that number and Product team was fine with it (see https://bugzilla.mozilla.org/show_bug.cgi?id=1095358#c2)
Assignee: nobody → frsela

Updated

3 years ago
Iteration: --- → 42.1 - Jul 13
Rank: 33
Flags: qe-verify+
Flags: firefox-backlog+
Priority: -- → P3
Whiteboard: [chat bug]
124 seems aribitrary yet reasonable. In the panel or room titlebar, the conversation name should trail off to elipses if it is too long.

I don't think we need to limit the number of characters to force room titles to fit.

As such, we can't just leave it open-ended in-case someone puts in a gigantic string which causes problems.

We can leave it as it, or even make it Twitter limit for fun: 140 characters.
Flags: needinfo?(sfranks)
Created attachment 8632030 [details]
snapshot102_chars.png

Setting 102 characteres to the converstaion name, server rejects renaming, so the practical limit is 101
That's strange. What we save on the server is encrypted context - there shouldn't be a length limit at all.
Hang on a minute, I think that's a screenshot of release which isn't using the context yet. Can you try nightly instead?
Created attachment 8632071 [details] [diff] [review]
Bug1117080.patch
Attachment #8632071 - Flags: feedback?(mdeboer)
(In reply to Mark Banner (:standard8) from comment #6)
> Hang on a minute, I think that's a screenshot of release which isn't using
> the context yet. Can you try nightly instead?

This version had been built by me this morning
Created attachment 8632092 [details] [diff] [review]
Bug1117080.patch
Attachment #8632092 - Flags: review?(standard8)
Attachment #8632071 - Attachment is obsolete: true
Attachment #8632071 - Flags: feedback?(mdeboer)
(In reply to Mark Banner (:standard8) from comment #5)
> That's strange. What we save on the server is encrypted context - there
> shouldn't be a length limit at all.

My last test (with patch) stored a 124 name:

1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234
Created attachment 8632098 [details] [diff] [review]
Bug1117080.patch
Attachment #8632092 - Attachment is obsolete: true
Attachment #8632092 - Flags: review?(standard8)
Attachment #8632098 - Flags: review?(standard8)
Comment on attachment 8632098 [details] [diff] [review]
Bug1117080.patch

Review of attachment 8632098 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8 with the eslint error fixed. The other one is just a comment.

::: browser/components/loop/content/js/roomViews.jsx
@@ +285,5 @@
>    });
>  
>    var DesktopRoomEditContextView = React.createClass({
>      mixins: [React.addons.LinkedStateMixin],
> +    maxRoomNameLength: 124,

I found there was an instance where we could use "statics" as part of the createClass definition, but it seems mainly for functions and I doubt it'd by us much, so I think defining like this is fine.

@@ +512,5 @@
>            <form onSubmit={this.handleFormSubmit}>
>              <input className="room-context-name"
>                onKeyDown={this.handleTextareaKeyDown}
>                placeholder={mozL10n.get("context_edit_name_placeholder")}
> +              maxLength={this.maxRoomNameLength}

Eslint says:

516:14  error  Props should be sorted alphabetically  react/jsx-sort-props
Attachment #8632098 - Flags: review?(standard8) → review+
Fernando: I've landed this for you with the change I asked for, as I know we're both away next week.
(In reply to Mark Banner (:standard8) from comment #14)
> Fernando: I've landed this for you with the change I asked for, as I know
> we're both away next week.

Thank you Mark !
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
QA Contact: anthony.s.hughes → bogdan.maris
Verified that conversation name can contain a maximum of 124 characters on latest Nightly across platforms (Windows 7 64-bit, Mac OS X 10.10 and Ubuntu 14.04 32-bit).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.