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

Auto suggest username #1022

Merged
merged 16 commits into from
Feb 29, 2024

Conversation

balsa-asanovic
Copy link
Contributor

@balsa-asanovic balsa-asanovic commented Jan 28, 2024

Problem

As described in issue #112 this PR addresses the suggestion of username for user account creation based on full name input.

Solution

Usernames are suggested based on the given full name in the following manner:

  • Just the first word of the given name
  • The first letter of the first word + all other full words
  • The full first word + first letters of all other words
  • First letters of all words except last + full last word
  • All words of a name joined without white spaces

Also, all suggestions with less than 3 characters are filtered out.

This PR fixes #112

Testing

  • Manually tested
  • Added unit tests in web/src/components/users/utils.test.js

Screenshots

image

Three suggestions are given:
name+surname
nameInitial+surname
name+surnameInitial
@@ -166,6 +172,26 @@ export default function FirstUser() {
setFormValues({ ...formValues, [name]: value });
};

const suggestUsernames = (fullName) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll start immediately with some uncertainties 😄

When the user types two words for the full name, these current suggestions work well.

However, if the user's full name has more than two words (names), the choice is not so clear.
People having longer full names is a common thing in some parts of the world (like Spain) and with the current approach to suggesting the username, Ronaldo Luís Nazário de Lima would be presented with the following choices:

image

The issues are apparent, the first suggestion is pretty long, the second might be as well and only the third one might be okay.

Should we maybe take only the first two words (names)?
Or maybe some other approach entirely, feel free to make a suggestion.

Copy link
Contributor

@dgdavid dgdavid Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, but I fear that guessing which are the right or expected parts is a bit difficult due to the fact on how different can be personal names around the world (long and old article).

Therefore, I wouldn't worry too much about them. They are just suggestions. Of course, the idea is to show up the more sensible ones, but we can't do so for every single user. I propose below as first. Some of them are based on feedback received in the issue, some are not:

  • Just the first word (the simplest approach)

    To make @kobliha happy

    • Lubos Kocman: lubos
    • Imobach González Sosa: imobach
    • Ronaldo Luís Nazário de Lima: ronaldo
  • First letter of first word plus second word

    It satisfies the original request from @lkocman

    • Lubos Kocman: lkocman
    • Imobach González Sosa: igonzalez
    • Ronaldo Luís Nazário de Lima: rluis
  • First letter of first word plus all other words

    It would suggest a username like the one @imobachgs have at SUSE

    • Lubos Kocman: lkocman
    • Imobach González Sosa: igonzalezsosa
    • Ronaldo Luís Nazário de Lima: rluisnazariodelima
  • First word plus first letter of the other words (maybe with a limit of just 2 more words?)

    • Lubos Kocman: lubosk
    • Imobach González Sosa: imobachgs
    • Ronaldo Luís Nazário de Lima: ronaldolndl (or ronaldoln if using a limit)
  • First letter of all words except the last one, which would be complete

    • Lubos Kocman: lkocman
    • Imobach González Sosa: igsosa
    • Ronaldo Luís Nazário de Lima: rlndlima
  • All words

    • Lubos Kocman: luboskocman
    • Imobach González Sosa: imobachgonzalezsosa
    • Ronaldo Luís Nazário de Lima: ronaldoluiznaariodelima

So, after removing duplicates, suggestions for these users will be,

Lubos Kocman Imobach González Sosa Ronaldo Luís Nazário de Lima
  • lubos
  • lkocman
  • lubosk
  • imobach
  • igonzalez
  • igonzalezsosa
  • imobachgs
  • igsosa
  • imobachgonzalezsosa
  • ronaldo
  • rluis
  • rluisnazariodelima
  • ronaldolndl (or ronaldoln)
  • rlndlima
  • ronaldoluiznaariodelima

I do not have strong preference and ideas from others are welcome. I think that we can release the feature with these few (or even only the three or four first ones) and wait for more suggestion requests.

BTW, as you can see in my examples above, I prefer not using the "special characters" in the suggested username (accents and friends). See https://stackoverflow.com/a/70288180 and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/normalize

dgdavid

This comment was marked as outdated.

@@ -166,6 +172,26 @@ export default function FirstUser() {
setFormValues({ ...formValues, [name]: value });
};

const suggestUsernames = (fullName) => {
const cleanedName = fullName.replace(/[^\p{L}\p{N} ]/gu, "").toLowerCase();
Copy link
Contributor

@dgdavid dgdavid Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: At least for me, that line deserves an explanation / comment 🤯

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm not sure if it pays off.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: At least for me, that line deserves an explanation / comment 🤯

Haha, indeed, this was weird for me as well.
This has almost identical effect as the regular expression /[^a-zA-Z0-9 ]/g, which cleans the string from all characters except alphanumeric and blank spaces.
However, when I just used this initially, non English characters (like š, ć, č, ...) would also get removed.

So, after googling for a bit, I found that since ECMAScript 2018 Unicode property escapes are available for regular expressions in JavaScript. So, the '\p{L}' matches any character from any language and '\p{N}' matches any numeric character.

Also, I'm not sure if it pays off.

Of course, it is another thing should something like this be used at all. When I coded this, the idea was that users could enter some special characters in their full names, like ',', '.', ';', '-', '"', etc,... And I supposed that this isn't something that should be a part of the username.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the explanation!

If we keep it in the code, definitely it deserves a comment above it.

However, I think that "johnq.diaz" is a valid username for "John Q. Díaz" where "johnqdíaz" is not. If I'm right, we can sanitize the name in another way or with other regular expression. Either way, a comment in the code is more than welcome for future developers.

Keep going!

@balsa-asanovic
Copy link
Contributor Author

Hey @dgdavid

Sorry for a bit of a delay with these changes.

I've been on a vacation the week before last and then last week had some catching up to do.

I think I've addressed all your comments, so please check it out when you find the time.

I've modified the suggestUsername method in accordance with your suggestions and moved it to utils.js file

dgdavid

This comment was marked as outdated.

@balsa-asanovic

This comment was marked as outdated.

@dgdavid
Copy link
Contributor

dgdavid commented Feb 27, 2024

How should the keyboard commands be handled here? When the user is focused on username input field, should the arrow down key press start going through the options of the dropdown list?

I think so. Plus Enter should select the suggestion, like the click does.

Or maybe use Tab?

No, Tab should respect the normal flow of the form and just jump to the next field.

@coveralls
Copy link

coveralls commented Feb 27, 2024

Coverage Status

coverage: 74.566% (+0.03%) from 74.532%
when pulling eb79c6f on balsa-asanovic:auto-suggest-username
into 0951b8c on openSUSE:master.

Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@balsa-asanovic,

I've had a short chat with @imobachgs about the state of this PR. We've conclude that it looks good to us as it is now :) 💪 🎉

We think that the keyboard support can be done as a follow-up PR, following the same approach we are trying to apply in the team to avoid long PRs. You can do it if you want to keep working in this area, or just open an issue for it and pick another one to work on if you prefer something new instead.

So, just adding the changes file is enough for merging it into master. Also, you can update/reword a bit the PR description if you don't mind, since it is a bit outdated if I'm not wrong.

Thanks a lot!

@dgdavid
Copy link
Contributor

dgdavid commented Feb 27, 2024

Sorry, not much knowledge about the accessibility topic 😅 That said, I do like how it looks and it sort of clears up any dilemma about what is being suggested in the dropdown

Ok, let's release it as it is and gather feedback from users.

@balsa-asanovic
Copy link
Contributor Author

Hay @dgdavid

First, as always thank you a lot for continued support and mentorship. 👏

You can do it if you want to keep working in this area, or just open an issue for it and pick another one to work on if you prefer something new instead.

Well, since I started working on this, I guess I would like to finish that part as well.
Should I open this issue even in the case that I'll be working on it?

So, just adding the changes file is enough for merging it into master. Also, you can update/reword a bit the PR description if you don't mind, since it is a bit outdated if I'm not wrong.

I've logged changes and also updated a bit the description of the PR

It's a test that have a lot of unknown words because they are username suggestions. It makes sense to disable the spell checker in this case.
Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 thanks a ton!

@dgdavid dgdavid merged commit 8946391 into agama-project:master Feb 29, 2024
1 check passed
@dgdavid
Copy link
Contributor

dgdavid commented Feb 29, 2024

Hay @dgdavid

First, as always thank you a lot for continued support and mentorship. 👏

Anytime!

Should I open this issue even in the case that I'll be working on it?

I don't think so, just reference this or #112 in the new PR description and/or the entry in the changes file.

I've logged changes and also updated a bit the description of the PR

And now it's merged :)

@balsa-asanovic balsa-asanovic deleted the auto-suggest-username branch March 22, 2024 22:20
dgdavid added a commit that referenced this pull request May 2, 2024
## Problem

In #1022 the username auto-suggest feature was introduced, but the
dropdown with usernames didn't support keyboard navigation.

This PR aims to solve this, It introduces navigation with arrow keys (up
and down) and sets the value on the press of Enter.

fixes #1070 

## Solution

Now checking for keyboard events on the username input field and on the
arrow presses changing the index of the selected menu item, highlighting
the appropriate one. On the press of Enter, again using the current
value of index, the text input is populated with a value from the
usernames array.
@imobachgs imobachgs mentioned this pull request May 17, 2024
imobachgs added a commit that referenced this pull request May 17, 2024
Prepare for releasing Agama 8. It includes the following pull requests:

* #884
* #886
* #914
* #918
* #956
* #957
* #958
* #959
* #960
* #961
* #962
* #963
* #964
* #965
* #966
* #969
* #970
* #976
* #977
* #978
* #979
* #980
* #981
* #983
* #984
* #985
* #986
* #988
* #991
* #992
* #995
* #996
* #997
* #999
* #1003
* #1004
* #1006
* #1007
* #1008
* #1009
* #1010
* #1011
* #1012
* #1014
* #1015
* #1016
* #1017
* #1020
* #1022
* #1023
* #1024
* #1025
* #1027
* #1028
* #1029
* #1030
* #1031
* #1032
* #1033
* #1034
* #1035
* #1036
* #1038
* #1039
* #1041
* #1042
* #1043
* #1045
* #1046
* #1047
* #1048
* #1052
* #1054
* #1056
* #1057
* #1060
* #1061
* #1062
* #1063
* #1064
* #1066
* #1067
* #1068
* #1069
* #1071
* #1072
* #1073
* #1074
* #1075
* #1079
* #1080
* #1081
* #1082
* #1085
* #1086
* #1087
* #1088
* #1089
* #1090
* #1091
* #1092
* #1093
* #1094
* #1095
* #1096
* #1097
* #1098
* #1099
* #1100
* #1102
* #1103
* #1104
* #1105
* #1106
* #1109
* #1110
* #1111
* #1112
* #1114
* #1116
* #1117
* #1118
* #1119
* #1120
* #1121
* #1122
* #1123
* #1125
* #1126
* #1127
* #1128
* #1129
* #1130
* #1131
* #1132
* #1133
* #1134
* #1135
* #1136
* #1138
* #1139
* #1140
* #1141
* #1142
* #1143
* #1144
* #1145
* #1146
* #1147
* #1148
* #1149
* #1151
* #1152
* #1153
* #1154
* #1155
* #1156
* #1157
* #1158
* #1160
* #1161
* #1162
* #1163
* #1164
* #1165
* #1166
* #1167
* #1168
* #1169
* #1170
* #1171
* #1172
* #1173
* #1174
* #1175
* #1177
* #1178
* #1180
* #1181
* #1182
* #1183
* #1184
* #1185
* #1187
* #1188
* #1189
* #1190
* #1191
* #1192
* #1193
* #1194
* #1195
* #1196
* #1198
* #1199
* #1200
* #1201
* #1203
* #1204
* #1205
* #1206
* #1207
* #1208
* #1209
* #1210
* #1211
* #1212
* #1213
* #1214
* #1215
* #1216
* #1217
* #1219
* #1220
* #1221
* #1222
* #1223
* #1224
* #1225
* #1226
* #1227
* #1229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

auto-suggestion of username done differently
4 participants