Allow longer usernames displayed as links.

I guess we need some basic testcoverage to know that truncating works as expected. Btw. you could directly replace it with Unicode::strlen() already.

I have tested the patch from #1, and it seems to works fine in nodes author, and in "Who's new" blocks, and it works fine.

@oskar_calvo : "Needs tests" is not just looking for "Works OK for me today".

It is "We need automated unit tests added to core code that prove that this change performs as needed, and can be reliably re-tested automatically for every single change that happens in the future"

That's the difference between "Needs (automated) Tests" and "Needs (some) Testing/verification". The difference between these two type of testing is sort of a big deal.

It would be cool if you got involved with coding tests to the point where you could *prove* the fix works, and not just offer anecdotal support. It's a big step, I know.

Confirming that "It works for me" is also really helpful input and does help move things forward, so thanks for that! - it counts, and encourages folk who want to do unit-testing that this is worthwhile.

I have added some tests for the username. Node author, comment author and the "who's new" block gets checked if the full username is present or not.

Thanks!

I've got mostly nitpicks, but I think there's also a subtle flaw in the current solution — an additional precise test for the expected truncated username would've picked that up, so it might be worthwhile adding an assertion like $this->assertText('<my really long user name cut off to 29 characters>…').

1. +++ b/core/modules/user/lib/Drupal/user/Tests/UserUsernameTest.php
   @@ -0,0 +1,118 @@
   + * Definition of Drupal\user\Tests\UserUsernameTest.
   

   s/Definition of Drupal/Contains \Drupal/

2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserUsernameTest.php
   @@ -0,0 +1,118 @@
   +  /**
   +   * The profile to install as a basis for testing.
   +   *
   +   * Using the standard profile to test username functionality.
   +   *
   +   * @var string
   +   */
   +  protected $profile = 'standard';
   

   Is it necessary to use the standard profile? It's preferable to not use that, because it's significantly slower than the testing profile.

3. +++ b/core/modules/user/lib/Drupal/user/Tests/UserUsernameTest.php
   @@ -0,0 +1,118 @@
   +      'description' => 'Tests username functionality e.g. truncating.',
   

   Missing comma after "functionality".

4. +++ b/core/modules/user/lib/Drupal/user/Tests/UserUsernameTest.php
   @@ -0,0 +1,118 @@
   +   * Tests author name display on nodes.
   ...
   +   * Tests author name on comments.
   

   Let's use consistent descriptions here.

5. +++ b/core/modules/user/lib/Drupal/user/Tests/UserUsernameTest.php
   @@ -0,0 +1,118 @@
   +  }
   +}
   

   Missing newline — we always ensure there is one newline before and after a method.

6. +++ b/core/modules/user/user.module
   @@ -639,8 +639,8 @@ function template_preprocess_username(&$variables) {
   +  if (drupal_strlen($name) > 30) {
   +    $name = drupal_substr($name, 0, 27) . '…';
   

   This uses the ellipsis character (…) rather than three periods (...) — great! :)

   But… doesn't that mean the substring of the 29 first characters should be taken, rather that of the first 27 characters?

Good point there, Wim. I changed the truncating logic to "29 plus the ellipsis". I also fixed the remarks in the test and switched to the testing profile. It's a lot faster now indeed, thanks!

10: longer_username.2266991-10.patch queued for re-testing.

#10: could you provide an interdiff next time? :)

Just a few more nitpicks, then I'll be able to RTBC this:

1. +++ b/core/modules/user/lib/Drupal/user/Tests/UserUsernameTest.php
   @@ -0,0 +1,127 @@
   +  protected $userShortName;
   ...
   +  protected $userLongName;
   

   Missing docblocks.

2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserUsernameTest.php
   @@ -0,0 +1,127 @@
   +  public static function getInfo() {
   ...
   +  public function setUp() {
   

   {@inheritdoc}

Fixed issues from #13.. hope this helps

#14 patch seems to have fixed the issue with the character cut-off change to 29+ellipsis of the user name.
I've verified it working on node list view and node view.

I did a quick review of this patch and i just find one coding standard nitpick:

+++ b/core/modules/user/lib/Drupal/user/Tests/UserUsernameTest.php
@@ -0,0 +1,143 @@
+  public static $modules = array(
+    'node',
+    'block',
+    'comment_test',
+    'user_test_views'
+  );

The last element of this array needs a comma at the end.

For everything else, looks good to me.

git ac https://drupal.org/files/issues/longer_username.2266991-14.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  4996  100  4996    0     0  17034      0 --:--:-- --:--:-- --:--:-- 22204
error: patch failed: core/modules/user/user.module:639
error: core/modules/user/user.module: patch does not apply

Needs a reroll

Reroll. Note the switch from drupal_substr to Unicode::truncate().

Added the missing comma.

Verified output for node author seems to be 28 characters +ellipsis

<span rel="schema:author">
Submitted by <span class="field field-node--uid field-name-uid field-type-entity-reference field-label-hidden" data-quickedit-field-id="node/1/uid/en/full"><a title="View user profile." href="/user/1" lang="" about="/user/1" typeof="schema:Person" property="schema:name" datatype="" content="s6f7a89244c0c857s6f7a89244c0c857s6f7a89244c0c857s6f7a89244c0" class="username">s6f7a89244c0c857s6f7a89244c0…</a>
</span>

Also 28 characters +ellipsis on comment form and who's new.

Result

<span class="field field-node--uid field-name-uid field-type-entity-reference field-label-hidden" data-quickedit-field-id="node/2/uid/es/full"><span lang="" about="/user/4" typeof="schema:Person" property="schema:name" datatype="" content="no012345678901234567890123456789si">no01234567890123456789012345…</span></span>

+++ b/core/modules/user/lib/Drupal/user/Tests/UserUsernameTest.php
@@ -0,0 +1,143 @@
+   * The user with the longer short username.

longer short username?

Updated patch.

Thank you @Sharique, here's the missing interdiff.

1. +++ b/core/modules/user/lib/Drupal/user/Tests/UserUsernameTest.php
   @@ -0,0 +1,143 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getInfo() {
   +    return array(
   +      'name' => 'Username functionality',
   +      'description' => 'Tests username functionality, e.g. truncating.',
   +      'group' => 'User',
   +    );
   +  }
   

   getInfo() is gone, please see https://www.drupal.org/node/2301125.

2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserUsernameTest.php
   @@ -0,0 +1,143 @@
   +    $this->assertText($this->userShortName->getUsername(), 'Untruncated username is found');
   ...
   +    $this->assertNoText($this->userLongName->getUsername(), 'Untruncated username is not found');
   +    $this->assertText(drupal_substr($this->userLongName->getUsername(), 0, 29) . '…', 'Truncated username is found');
   ...
   +    $this->assertText($this->userShortName->getUsername(), 'Untruncated username is found');
   ...
   +    $this->assertNoText($this->userLongName->getUsername(), 'Untruncated username is not found');
   +    $this->assertText(drupal_substr($this->userLongName->getUsername(), 0, 29) . '…', 'Truncated username is found');
   ...
   +    $this->assertText($this->userShortName->getUsername(), 'Untruncated username is found');
   +    $this->assertNoText($this->userLongName->getUsername(), 'Untruncated username is not found');
   +    $this->assertText(drupal_substr($this->userLongName->getUsername(), 0, 29) . '…', 'Truncated username is found');
   

   Assertion messages usually are complete sentences (add a period to the end). I would also suggest that removing the word "is" from all these might be an improvement.

Forgot the attachment.

Incorporated #24 in this patch. Please review this patch.

@pgautam - interdiff please!

@Cottser, sorry for that. here is updated patch with interdiff file.

No need to apologize @pgautam just to good to get in the habit :) I cancelled testing on the first patch.

At a glance the changes look good. Thank you!

I've reviewed the patch(es) and found that #26 and #28 were both missing changes. I've created a new patch combining #26 and #28.

The UserUsernameTest.php file is placed in the core/modules/user/lib/Drupal/user/Tests directory, but should be in the core/modules/user/src/Tests directory.

I'm working on a reroll.

Rerolled with the test in core/modules/user/src/Tests.

The patch applies cleanly and the use case works. It's good for me to know that UserUsernameTest.php should be in core/modules/user/src/Tests although others should confirm.

I'll have a look at the failing test

The tests failed because they had several errors which I've fixed and tested thoroughly.

+++ b/core/modules/user/src/Tests/UserUsernameTest.php
@@ -0,0 +1,146 @@
+    $this->container->get('comment.manager')->addDefaultField('node', 'article');

Is this standard practice to depend on the container in tests? Can we make the node commentable in another way?

If this is the only method of creating fields quickly in tests, then OK.

It looks like core does this a lot in tests.

Maybe add some screen shots and update the issue summary.

Working in this issue.

Need reroll

Re-roll

Here's some failing tests to look at locally to debug why they're crashing. There are more fails than those listed in the review log section, but these aren't making it through their entire test. Most of those tests are in Comment and Search modules. Try running a test like CommentTitleTest locally.

+++ b/core/modules/user/user.module
@@ -528,8 +528,8 @@ function template_preprocess_username(&$variables) {
+  if (drupal_strlen($name) > 30) {

Unicode::strlen() should still be used here, not drupal_strlen().

reroll.

Thanks @mradcliffe for review :)

It works for me.

I would have expected some screenshots of the admin/people screen and other pages with user names displayed, eg. comments, nodes and user pages.

Hi, thanks for review.

Here screenshot. I think with this is correct.

Info:

• node -->1
• comment -->1
• user "admin" -->1
• user "test user 0123456789 012345 looks well user name test" -->5

Good stuff, thanks @keopx

Sorry, I think there is something wrong with that screeshots

Here correct screenshot, sorry with confusion :-$

Sorry @diego21

It's ok @keopx, changed state to RTBC

Thanks!

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Also the title needs fixing because we are not displaying the full username.

.

+++ b/core/modules/user/src/Tests/UserUsernameTest.php
@@ -0,0 +1,146 @@
\ No newline at end of file

Needs a newline at end of file.

correct mistake

See https://www.drupal.org/node/2411239

Rerolled the patch with test failure fixes. Hope this will work. :)

+++ b/core/modules/user/src/Tests/UserUsernameTest.php
@@ -18,6 +19,8 @@
+  use CommentTestTrait;

It is added already, so no need to add it again.

Drupal is using php's trait strategy. Check out #2134513: [policy, no patch] Trait strategy.

We need to add use Trait to make it working. Working without this cause test to fail.

+++ b/core/modules/user/src/Tests/UserUsernameTest.php
@@ -0,0 +1,149 @@
+use Drupal\comment\Tests\CommentTestTrait;
...
+  use CommentTestTrait;

I mean CommentTestTrait is used in "Use" statement is used twice. Please remove second one.

Oh, so this is how traits works.

+++ b/core/modules/user/src/Tests/UserUsernameTest.php
@@ -50,8 +53,8 @@ class UserUsernameTest extends WebTestBase {
+    $this->drupalCreateContentType(array('type' => 'article', 'name' => 'Article'));

Optional: Should we go with the shorthand array syntax? Here and other places in the patch.

Also, I have unfrozen this issue.

@lokapujya I do not understand your comment, I see other test and use this same line.

Works fine for me.

Here reroll with changed method .

Old: String::checkPlain
New: SafeMarkup::checkPlain

Changed to support new reserved php class names.

Here reroll with changed method .

Old: String::checkPlain
New: SafeMarkup::checkPlain

Changed to support new reserved php class names.

PS: sorry, because I forget some lines

I do not but in local enviroment test works :-/

#79 Patch worked fine for me but seems patch was failing because of interdiff.

Changed patch and interdiff according to #68. Lets give it a try.

Fixed a missing period. I can confirm that tests pass in local so let's see what's wrong with testbot.

I just tried to test it on my local machine and it returns fails (just like testbot):

Fail      Other      UserUsernameTest.  145 Drupal\user\Tests\UserUsernameTest-
    Untruncated username found.
Pass      Other      UserUsernameTest.  146 Drupal\user\Tests\UserUsernameTest-
    Untruncated username not found.
Fail      Other      UserUsernameTest.  147 Drupal\user\Tests\UserUsernameTest-
    Truncated username found.

I will look at it.

Let's try it again, this time without block cache.

If it's green, then it's RTBC for me.

Long Username BEFORE

Long Username AFTER

Thanks for the beta evaluation, test coverage, manual testing, and screenshots! I'd consider this a task -- it is altering an existing bit of functionality, not adding a new one. I think it makes sense to increase this default length as 15 is so short as to be a bit unfriendly. I've had to override this on sites before.

1. +++ b/core/modules/user/src/Tests/UserUsernameTest.php
   @@ -0,0 +1,149 @@
   +      $this->randomString(30)
   ...
   +      $this->randomString(31)
   ...
   +    $this->assertText(SafeMarkup::checkPlain(Unicode::truncate($this->userLongName->getUsername(), 29, FALSE, TRUE)), 'Truncated username found.');
   ...
   +      'comment_body[0][value]' => $this->randomMachineName(20),
   ...
   +      'comment_body[0][value]' => $this->randomMachineName(20),
   ...
   +    $this->assertText(SafeMarkup::checkPlain(Unicode::truncate($this->userLongName->getUsername(), 29, FALSE, TRUE)), 'Truncated username found.');
   

   So even if we choose not to make this default truncation length a constant or configurable, can we at least add a comment in the test that explains where this magic integer is set? (That is, the preprocess implementation.)

2. +++ b/core/modules/user/src/Tests/UserUsernameTest.php
   @@ -0,0 +1,149 @@
   +    $this->assertText(SafeMarkup::checkPlain($this->userShortName->getUsername()), 'Untruncated username found.');
   +    $this->assertNoText(SafeMarkup::checkPlain($this->userLongName->getUsername()), 'Untruncated username not found.');
   

   These assertion texts are very confusing. "Untruncated username found" is asserted immedidately after "untruncated username not found"! In the code it's clear that it's two different names.

   I think it'd actually be better to remove the assertion message texts from all the similar assertions in this new test class. assertText() has a default assertion message that provides more helpful and specific information for debugging than this. In this case the assertion texts are hiding information.

3. +++ b/core/modules/user/user.module
   @@ -459,8 +459,8 @@ function template_preprocess_username(&$variables) {
   -  if (Unicode::strlen($name) > 20) {
   -    $name = Unicode::truncate($name, 15, FALSE, TRUE);
   +  if (Unicode::strlen($name) > 30) {
   +    $name = Unicode::truncate($name, 29, FALSE, TRUE);
   

   I considered whether this should be a constant or something configurable rather than a magic integer, but since this is a preprocess anyway and can be overridden as desired, that's probably not necessary. The comment above this hunk also documents that it's the specific preprocess's responsibility to truncate safely to the desired length.

   However, the 30 and 29 do seem a bit arbitrary. Would it be clearer to have a variable for the length, and truncate to length - 1?

   Also, I think the previous 15 vs. 20 was to leave room for the characters of the ellipsis. So now a 31-character username will have its length increased to 32. No? Let's at least document the lengths chosen.

My only other concern is that this might be very wide in existing tables where the user name is a high priority field, for example the user admin view. Can we get additional screenshots of long, truncated usernames in that view, both at a desktop width and a mobile width? Embed the screenshots as @rteijeiro has or (even better!) in the issue summary. (Also the summary looks a bit out of date.)

Thanks!

Thanks

Kudos for @xjm and @rteijeiro

1. +++ b/core/modules/user/user.module
   @@ -45,6 +45,16 @@
   + *
   + */
   ...
   +
   +/**
   

   This change looks wrong.

2. +++ b/core/modules/user/user.module
   @@ -45,6 +45,16 @@
   + *
   

   Please, document the constant. For example: Trimming length for long usernames.

Shouldn't USERNAME_TRIMM_LENGTH be with just one M?

USERNAME_TRIMM_LENGTH-1 is to include the ellipsis character (which used to be 3 periods.

Update comments.

Looks good.

Thanks @rteijeiro for the awesome screenshots :D But the reason I specified the user admin view is this:

So based on the fact that we're breaking an existing UI a little with this change, I think this needs more discussion.

I also discussed the class constant with @amateescu and @dawehner. Rather than adding a global constant, they suggested adding as a variable in user_theme(). Then I think it would be easier for themes to override it and still get the proper truncation, but that also gives us a place to document the quantity.

Moving to get this tackled in 8.1.x version since we are in RC.

needs a reroll + see #111 (about moving the constant to a variable in user_theme().

Patch from #109 re-rolled

Looks like the novice tag was added back in 2014, when core was in a different state. Since there isn't a clear novice task on this issue per https://www.drupal.org/core-mentoring/novice-tasks, I am removing the novice tag.

First 2 test fails: user_format_name_alter() doesn't get called anymore because the of this change:

+++ b/core/modules/user/user.module
@@ -470,10 +475,9 @@ function template_preprocess_username(&$variables) {
-  $name  = $account->getDisplayName();
...
+  $name = $variables['name_raw'] = $account->getUsername();

getDisplayName invokes the hooks, but getUserName does not.

The test is relying on the alter hook to run and change the name to an ID.

So, we aren't really done with the reroll.

Patch re-rolled and used variables to allow themer to overriding.

These tests have been failing is what seems to be an unrelated area

exception: [Uncaught exception] Line 98 of core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php:
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for block.block.zjxrwbkj with the following errors: block.block.zjxrwbkj:settings.cache missing schema in Drupal\Core\Config\Testing\ConfigSchemaChecker->onConfigSave() (line 98 of /var/www/html/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php). Drupal\Core\Config\Testing\ConfigSchemaChecker->onConfigSave(Object, 'config.save', Object) (Line: 116)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('config.save', Object) (Line: 232)

This has been happening on our local dev environments, and Now I am seeing it in the testbot report too.

I can't see what the connection is between "ConfigSchemaChecker" and what we are testing here.

Changing string validation in patch based on testbot output.

Something is wrong with interdiff. @th_tushar please work as mentioned by @dman

@sivaji@knackforge.com
You can create an interdiff correctly following the steps in this url:
https://www.drupal.org/documentation/git/interdiff

I hope you find it useful.

Hey,

The issue will be fixed with the updated patch. Changing the status of the issue to "Needs Review".

Thanks,
th_tushar

Could you please add an interdiff?

Hey,

Reviewed the interdiff file in #132, its fine. Its solves the issues with the submitted patch in #129.

Changing the issue status to "Need Review".

Thanks,

Reverse the change made in the reroll described in #118.
Remove some changes that seem unnecessary.

The new patch that fixes all the issues.

@th_tushar the interdiff in #132 IS the interdiff between #125 and #129. It shows the changes that you made. You should provide an interdiff with each patch that you upload so that we can see the changes between patches.

+++ b/core/modules/user/src/Tests/UserUsernameTest.php
@@ -0,0 +1,164 @@
+
+use Drupal\Core\Url;
+//use Drupal\views\Entity\View;
+use Drupal\views\Views;
+use Drupal\views\Tests\ViewTestBase;
+//use Drupal\Core\Template\Attribute;
+use Drupal\Component\Utility\SafeMarkup;
+use Drupal\Component\Utility\Unicode;

Is there a reason why are we commenting these lines instead of deleting them?

And Yes, we need a new interdiff for every new patch.

Hi All,

I have removed the commented code and added the interdiff.txt file in comparison to patch added in #125. The attached patch fixes all the issue.

Thanks,
th_tushar

Yes, patch is working as expected

I reviewed the patch and its working as expected.

1. +++ b/core/modules/comment/src/Tests/CommentPreviewTest.php
   @@ -50,13 +50,13 @@ function testCommentPreview() {
   -    $this->assertEscaped('<em>' . $this->webUser->id() . '</em>');
   +    $this->assertNoEscaped('<em>' . $this->webUser->id() . '</em>');
   ...
   -    $this->assertRaw('<em>' . $this->webUser->id() . '</em>');
   +    $this->assertNoRaw('<em>' . $this->webUser->id() . '</em>');
   
   +++ b/core/modules/tracker/src/Tests/TrackerTest.php
   @@ -216,13 +216,13 @@ function testTrackerUser() {
   -    $this->assertEscaped('<em>' . $this->user->id() . '</em>');
   +    $this->assertNoEscaped('<em>' . $this->user->id() . '</em>');
   ...
   -    $this->assertRaw('<em>' . $this->user->id() . '</em>');
   +    $this->assertNoRaw('<em>' . $this->user->id() . '</em>');
   

   We should not be changing these tests.

2. +++ b/core/modules/user/src/Tests/UserUsernameTest.php
   @@ -7,17 +7,19 @@
   +use Drupal\Core\Url;
   +use Drupal\views\Views;
   +use Drupal\views\Tests\ViewTestBase;
   ...
   -use Drupal\simpletest\WebTestBase;
   ...
   -class UserUsernameTest extends WebTestBase {
   +class UserUsernameTest extends ViewTestBase {
   

   Why is this being changed to a viewstest?

3. +++ b/core/modules/user/src/Tests/UserUsernameTest.php
   @@ -65,7 +67,7 @@ public function setUp() {
   -      $this->randomString(30)
   +      $this->randomMachineName(25)
   
   @@ -77,8 +79,10 @@ public function setUp() {
   -      $this->randomString(31)
   +      $this->randomMachineName(35)
   

   why changing the string sizes? Why using machine name (it's a user name not a machine name)?

4. +++ b/core/modules/user/src/Tests/UserUsernameTest.php
   @@ -100,8 +104,8 @@ public function testTruncatedNodeAuthorName() {
   -    $this->assertTrue(SafeMarkup::isSafe($this->userShortName->getUsername()), 'Username is marked safe');
   ...
   +    $this->assertTrue($this->userShortName->getUsername(), 'Username is marked safe');
   

   Why removing the isSafe() if the assert text says "marked safe"?

I have fixed all these issues in the #134 patch.

#141 looks great!
Thanks!

Just one very small nitpick.

+++ b/core/modules/user/src/Tests/UserUsernameTest.php
@@ -0,0 +1,158 @@
+    $this->assertNoText(SafeMarkup::checkPlain($this->userLongName->getUsername()));
+    $this->assertText(SafeMarkup::checkPlain(Unicode::truncate($this->userLongName->getUsername(), USERNAME_TRIM_LENGTH - 1, FALSE, TRUE)));
+  }
+
+
+  /**
+   * Tests username display in Who's New block.
+   */

Let's remove one empty line here.

Also, all the new patches to I've reviewed lately use the new short array syntax in new functions and methods, can we use that syntax here as well?

also removed some deprecated safemarkup calls.

Some final nits:

1. +++ b/core/modules/user/src/Tests/UserUsernameTest.php
   @@ -0,0 +1,158 @@
   +    // Tests if the username is trimmed to USERNAME_TRIM_LENGTH value.
   +    // @see template_preprocess_username().
   +    $this->assertTrue(Html::escape($this->userLongName->getUsername()));
   ...
   +    $this->drupalPostForm('comment/reply/node/' . $node->id() . '/comment', $comment, t('Save'));
   +    $this->assertTrue(Html::escape($this->userLongName->getUsername()));
   +    $this->drupalLogin($this->userLongName);
   

   Those assertTrue are repeated, even if they are in different methods the $this->userLongName is defined in the setup, so I think we can remove one.

2. +++ b/core/modules/user/src/Tests/UserUsernameTest.php
   @@ -0,0 +1,158 @@
   +
   +    $this->drupalLogin($this->userShortName);
   +    $comment =[
   +      'subject[0][value]' => $this->randomMachineName(),
   

   There is a missing space after the "=" operator in the array.

3. +++ b/core/modules/user/src/Tests/UserUsernameTest.php
   @@ -0,0 +1,158 @@
   +    // @see template_preprocess_username().
   

   I think the @see statements no need the period in the end.

Can we update the description of the issue? I think the "remaining task" is outdated.

This looks great! Thanks!

1.) Looks like those assertTrue()'s should be assertText()s.

The assertTexts might fail.

Great work!

Shouldn't it be more smarter to trim username without SPACE only ?

@droplet: can you elaborate on #152? I don't completely get it. Are you suggesting we trim whitespace after trimming the length?

We can't ignore #111 either. The theme may need an update to keep edit buttons visible on mobile?

@droplet I'm not sure if that would be smarter, even if the username has an space it could be something like:

"this isasuperlongusernamewithanspace"

Even with an space this username would need to be trimmed.

But what if the space is the last character after trimming? That could be a separate issue though (if that's what he meant.)
"this ..." vs.
"this..."

@gnuget,

ahh, you right :)

The case of "this ..." will not happen because Unicode::PREG_CLASS_WORD_BOUNDARY is used while detecting the stripped string.
@see: https://github.com/drupal/drupal/blame/8.1.x/core/lib/Drupal/Component/U...

This is good to know!

Thanks!

It seems to the testbot fail for an unknown reason, I will test it again just to be sure if something is wrong.

So, the remaining task is to make the operation links visible in mobile, see comment #111. How do we do this?

+++ b/core/modules/user/user.module
@@ -469,14 +469,18 @@ function template_preprocess_username(&$variables) {
+  if (!isset($variables['trim_length'])) {
+    $variables['trim_length'] = USERNAME_TRIM_LENGTH;
+  }

In #120, the $variables['trim_length'] was added. Is there actually a way for a theme (such as the Seven theme) to set this variable?

Try to fix the remaining task on #161, where says that we have to look on #111.

Before:

After:

+++ b/core/themes/seven/css/components/form.css
@@ -290,6 +290,14 @@ select {
+  form[action="/admin/people"] table {
+    table-layout: fixed;
+  }
+  form[action="/admin/people"] table td.views-field-name {
+    white-space: nowrap;
+    overflow: hidden;
+    text-overflow: ellipsis;
+  }

I looked at the code if there are other places where the CSS is just for a specific form action and I haven't found anything.

So maybe this is not a good approach to fixing this?

why not @gnuget?

This only happens in this case, you will never change the route and is the best solution that can be given from the visuals no other functionality of other modules and pages undergo this modification.

A more common way might be to target the ID of the form (for one off CSS)?

why not @gnuget?

The action is not guaranteed to be the same. For example, would it change in another language? What if an alias is used? etc..

why not @gnuget?

According to our guidelines:

Common CSS Pitfalls #

To better understand the best practices provided below, it can be helpful to review some common approaches that impede our goals of predictability, maintainability, reusability and scalability.

Pitfall: Modifying components based on context #

/* Modifying a component when it’s in a sidebar. */
.sidebar .component {}
This may seem natural, but actually makes CSS less predictable and maintainable. Sooner or later you’re going to need that component style somewhere other than a sidebar! Or, the reverse may happen: a new developer places a component in the sidebar and gets an unexpectedly different appearance.

Pitfall: Relying on HTML structure #

Mirroring a markup structure in our CSS selectors makes the resulting styles easy to break (with markup changes) and hard to reuse (because it’s tied to very specific HTML).

So, I think we are here modifying a component based in context AND relying on HTML structure, so yes definitely this is not the correct approach.

You can read more about this here: https://www.drupal.org/coding-standards/css/architecture

Thank you @gnuget & @lokapujya. What is the next step you want me to do?

Remove "action" attribute in css file?

@nesta_ You can implement how ever you think is best and we will review. I think the CSS needs a class or a form ID selector.

What happens when NOT in mobile view. A new patch should maybe have a screenshot of both mobile and non mobile. I would have to know more about Bartik to give you a better answer. It possibly needs a media query (if it's not already in one, can't tell from the patch)?

Separately, we still have to answer the 2nd question in #161.

Add test against 8.3.x on #164.