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

[web] Adds a core/Selector component #1012

Merged
merged 18 commits into from
Feb 15, 2024
Merged

[web] Adds a core/Selector component #1012

merged 18 commits into from
Feb 15, 2024

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Jan 24, 2024

Adds a core/Selector component and uses it to render suitable selectors across Agama codebase instead of defining almost the same code repeatedly.

Note that, while it was originally intended to have a fully accessible selector, time and priority constraints (among other reasons) forced to just centralize the repetitive code for rendering selectors with an small approach change.

Once priorities permit, work on a better, fully accessible selector will be resumed.


Adapted selectors:

  • L10n: Language, Keymap, Timezone
  • Storage: Disk selectors, Space policy
  • Product: product selection
Toggle screenshots
Before After
Screen Shot 2024-01-25 at 11 04 50 Screen Shot 2024-01-25 at 11 03 15
Screen Shot 2024-01-25 at 11 06 13 Screen Shot 2024-01-25 at 11 05 37
Screen Shot 2024-01-25 at 11 06 46 Screen Shot 2024-01-25 at 10 58 05
Screen Shot 2024-02-10 at 01 01 38 Screen Shot 2024-02-10 at 01 00 03
Screen Shot 2024-02-10 at 01 01 32 Screen Shot 2024-02-10 at 00 59 54
Screen Shot 2024-02-10 at 01 01 48 Screen Shot 2024-02-10 at 01 00 23
Screen Shot 2024-02-14 at 12 22 44 Screen Shot 2024-02-10 at 01 29 16

Related to #786 (comment)

@coveralls
Copy link

coveralls commented Jan 24, 2024

Coverage Status

coverage: 74.031% (+0.02%) from 74.011%
when pulling a7a88a5 on better-selector
into 9b6654b on master.

<LocaleSelector locales={locales} aria-label="Available locales" />
);

const filterInput = screen.getByRole("search");
Copy link
Contributor Author

@dgdavid dgdavid Jan 24, 2024

Choose a reason for hiding this comment

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

⚠️ NOTE: This made me realize that Agama is doing wrong in terms of accessibility and usability by not using proper <label>s for no other reason than visual aesthetic. This test example is working just because there is only one input with the "search" role, but it would fail trying to get it by its name/label too, which helps to reveal the underlying problem:

-   const filterInput = screen.getByRole("search");
+   const filterInput = screen.getByRole("search", { name: /Filter by language/ } );
TestingLibraryElementError: Unable to find an accessible element with the role "search" and name `/Filter by language/`

    Here are the accessible roles:

      search:

      Name "":
      <input
        placeholder="Filter by language, territory or locale code"
        role="search"
        type="text"
      />

      --------------------------------------------------

Of course, taking a look at the UI, for us and thanks to the placeholder it's clear the input functionality

Screenshot from 2024-01-24 19-41-52

but it might not be as obvious to other users.

Something to work on. But in a different PR.


From W3c (https://www.w3.org/WAI/tutorials/forms/instructions/#placeholder-text)

While placeholder text provides valuable guidance for many users, placeholder text is not a replacement for labels

From MDN (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-placeholder)

Warning: Using a placeholder instead of a visible label harms accessibility and usability for many users including older users and users with cognitive, mobility, fine motor skill and vision impairments. Labels are better: they are always visible and they provide for a larger hit region to focus on the control.
...
Note: Placeholders should only be used to show an example of the type of data that should be entered into a form; they don't replace a proper label.

Other links,

https://www.w3.org/WAI/GL/low-vision-a11y-tf/wiki/Placeholder_Research


/cc @joseivanlopez, @imobachgs

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! Definitely, "innovative" UI approaches are quite ... dangerous :)

@dgdavid dgdavid force-pushed the better-selector branch 2 times, most recently from dc39b57 to e4bc9a4 Compare January 25, 2024 11:07
@joseivanlopez

This comment was marked as outdated.

@dgdavid

This comment was marked as outdated.

Despite being in a preliminary state, it is a good start to unify all
the selectors. Also, it is the first step toward switching from the
Listbox[1] to Grid[2] pattern, because selector options now include at
least one interactive element: a radio button when mounted as a single
selector or a checkbox when multiple selection is allowed.

Grid pattern implementation (or its adoption, if finally the project
ends up using a third-party dependency) will take a long time, but sadly
it isn't expected to be completed in the near future[3].

Hence, this commit is part of a request to centralize selectors base
code and be ready for better implementation when time and priorities
permit.

[1] https://www.w3.org/WAI/ARIA/apg/patterns/listbox/
[2] https://www.w3.org/WAI/ARIA/apg/patterns/grid/
[3] #786 (comment)
TypeScript complained about:

> src/components/core/Selector.jsx:115:41 - error TS2339: Property 'props' does not exist on type 'ReactNode'.
> 115       { React.Children.map(children, ({ props: { id, key, children } }) => {

And it was right because a React child could be something without
`props`, like undefined or just a plain string[1]. Thus, the easy way of
protecting the code and pleasing TypeScript is checking that that child
is a valid React element.[2]

However, this is yet another hint warning about that maybe the selector
should stop doing a children manipulation and goes for a
`options`/`renderOption` props or simila approach instead, although it
defeats the idea of having an expressive JSX-based markup.

[1] https://react.dev/reference/react/Children#children-map-caveats
[2] https://react.dev/reference/react/isValidElement#reference
Because it will result in `undefined`, as the React complain states

>  console.error
>    Warning: Option: `key` is not a prop. Trying to access it will
>      result in `undefined` being returned. If you need to access the same
>      value within the child component, you should pass it as a different
>      prop. (https://reactjs.org/link/special-props)
Please, be aware that changes sent in this commit are not intended to be
permanent since almost the whole involved logic will be reworked to
accomplish what is stated #1031.

Thus, there are things that could be done better but postponed on
purpose to be addressed in another request.
However, missing tests haven't been added because this selector is
subject to major changes in the short term according to
#103.
By using the `options` and `renderOptions` props for rendering the
selector children instead of directly defining them via JSX. At least
for now, this looks more convenient for this component since it was
using the React.Children API for "injecting" more content into
children, performing an extra iteration to do so.
@dgdavid dgdavid changed the title [WIP][web] Adds a core/Selector component [web] Adds a core/Selector component Feb 14, 2024
@dgdavid dgdavid marked this pull request as ready for review February 14, 2024 12:23
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

LGTM

@dgdavid dgdavid merged commit be9bb78 into master Feb 15, 2024
2 checks passed
@dgdavid dgdavid deleted the better-selector branch February 15, 2024 09:34
@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.

3 participants