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

improve patterns selector #1248

Merged
merged 4 commits into from
May 23, 2024
Merged

improve patterns selector #1248

merged 4 commits into from
May 23, 2024

Conversation

jreidinger
Copy link
Contributor

@jreidinger jreidinger commented May 23, 2024

Problem

There is broken counter how in search box for pattern selector. Also there is no clear indication that nothing is matched.

Solution

See screenshots how it is improved:

Old:
counter:
broken_counter

empty result:
broken_empty

New:
counter:
new_counter

empty result (attempt 1):
new_empty

empty result (attempt 2):
new_empty

@@ -166,6 +166,12 @@ function PatternSelector({ patterns, onSelectionChanged = noop }) {
);
});

if (selector.length === 0) {
selector = (
<Label>{_("No results")}</Label>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'would not use the <Label> component for this kind of things. Not even the Patternfly guys would use it either 😉

Not saying we have to follow strictly their design guidelines, but in this case I'm more aligned with them.

Use just plain old HTML like <span>, <b> or even <strong> if we considered it is something should be emphasise.

Also, do not hesitate of adding more text to make it clear. I cannot help too much with texts, but "No matches" looks a bit poor to me.

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.

I just added to NP comments for the record, not asking for changing them now.

Thanks for taking care.

web/src/components/software/PatternSelector.jsx Outdated Show resolved Hide resolved
Comment on lines 169 to 173
if (selector.length === 0) {
selector = (
<b>{_("None of the patterns match the text.")}</b>
);
}
Copy link
Contributor

@dgdavid dgdavid May 23, 2024

Choose a reason for hiding this comment

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

NP:

Sorry, I didn't realize before, but it is actually not needed to redefine the selector variable. You can just change {selector} to {selector.length ? selector : <NoResultFoundThing />} below. But don't worry and keep it as it is to minimize the conflicts with the new-ui branch.

Co-authored-by: David Díaz <1691872+dgdavid@users.noreply.github.com>
@jreidinger jreidinger merged commit 5505918 into master May 23, 2024
2 checks passed
@jreidinger jreidinger deleted the fix_patterns branch May 23, 2024 08:50
@imobachgs imobachgs mentioned this pull request Jun 27, 2024
imobachgs added a commit that referenced this pull request Jun 27, 2024
Prepare for releasing Agama 9. It includes the following pull requests:

- #1101
- #1202
- #1228
- #1231
- #1236
- #1238
- #1239
- #1240
- #1242
- #1243
- #1244
- #1245
- #1246
- #1247
- #1248
- #1249
- #1250
- #1251
- #1252
- #1253
- #1254
- #1255
- #1256
- #1257
- #1258
- #1259
- #1260
- #1261
- #1264
- #1265
- #1267
- #1268
- #1269
- #1270
- #1271
- #1272
- #1273
- #1274
- #1279
- #1280
- #1284
- #1285
- #1286
- #1287
- #1288
- #1289
- #1290
- #1291
- #1292
- #1293
- #1294
- #1295
- #1296
- #1298
- #1299
- #1300
- #1301
- #1302
- #1303
- #1304
- #1305
- #1306
- #1307
- #1308
- #1309
- #1310
- #1311
- #1312
- #1313
- #1314
- #1315
- #1316
- #1317
- #1318
- #1319
- #1320
- #1321
- #1322
- #1323
- #1324
- #1325
- #1326
- #1328
- #1329
- #1331
- #1332
- #1334
- #1338
- #1340
- #1341
- #1342
- #1343
- #1344
- #1345
- #1348
- #1349
- #1351
- #1352
- #1353
- #1354
- #1355
- #1356
- #1357
- #1358
- #1359
- #1360
- #1361
- #1362
- #1363
- #1365
- #1366
- #1367
- #1368
- #1371
- #1372
- #1374
- #1375
- #1376
- #1379
- #1380
- #1381
- #1383
- #1384
- #1385
- #1386
- #1387
- #1388
- #1389
- #1391
- #1392
- #1394
- #1395
- #1397
- #1398
- #1399
- #1400
- #1403
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.

2 participants