Reimplement Checkbox #6528

foxbunny · 2024-09-09T04:34:43Z

Replace SUI Checkbox with a simpler component so that it can be programmatically focused on error.

foxbunny · 2024-09-09T07:01:28Z

@ThiefMaster Regarding the failing test, I'm able to suppress it by doing this:

diff --git a/indico/web/client/js/react/forms/index.js b/indico/web/client/js/react/forms/index.js
index e5f39b23d1..dce0e902f8 100644
--- a/indico/web/client/js/react/forms/index.js
+++ b/indico/web/client/js/react/forms/index.js
@@ -5,8 +5,10 @@
 // modify it under the terms of the MIT License; see the
 // LICENSE file for more details.
 
+import * as fields from './fields';
+
 export {handleSubmissionError} from './errors';
-export {
+export const {
   FinalCheckbox,
   FinalComboDropdown,
   FinalDropdown,
@@ -17,7 +19,7 @@ export {
   FinalTextArea,
   FormFieldAdapter,
   unsortedArraysEqual,
-} from './fields';
+} = fields; // Fix circular dependency issue
 export {default as validators} from './validators';
 export {default as parsers} from './parsers';
 export {default as formatters} from './formatters';

It appears to be some circular dependency, or something along those lines, but I can't for the love of god figure out what it is. I ran madge before and after this change and there's no difference, so it might be something else.

I'd appreciate some help troubleshooting this.

ThiefMaster

Works fine! :)
I'll have a look at the weird test failure.

ThiefMaster · 2024-09-09T07:46:21Z

CHANGES.rst

 ^^^^^^^^^^^^^

 - Improve registration form single choice input accessibility (:pr:`6310`, thanks :user:`foxbunny`)
+- Reimplement Checkbox to make it programmatically focusable (:pr:`6528`, thanks :user:`foxbunny`)


Please rebase and add it to the 3.3.5 section since 3.3.4 has been released last week

indico/modules/events/registration/client/js/form/fields/MultiChoiceInput.jsx

ThiefMaster · 2024-09-09T08:48:03Z

indico/web/client/js/react/components/Checkbox.module.scss

+  align-items: center;
+}
+
+.checkbox {


I had a look in the room booking admin UI where we use quite a few FinalCheckbox components w/ a description below, and the margins seem to be a bit different from other fields. Could you try to align them a bit more with what we have for those other fields? In the screenshot (the page URL is /rooms/admin if you want to go there yourself) it's quite easy to see:

ThiefMaster · 2024-09-09T08:54:07Z

CHANGES.rst


+- Reimplement Checkbox to make it programmatically focusable (:pr:`6528`, thanks :user:`foxbunny`)
+
 - Nothing so far :(


that one is meant to be removed when adding the first real changelog line :p

Sorry, didn't notice it.

ThiefMaster · 2024-09-09T11:07:19Z

Easy fix for the circular import :) (but don't ask me why this fixes it)

diff --git a/indico/web/client/js/react/forms/fields.jsx b/indico/web/client/js/react/forms/fields.jsx
index b8ae286405..47cab07161 100644
--- a/indico/web/client/js/react/forms/fields.jsx
+++ b/indico/web/client/js/react/forms/fields.jsx
@@ -12,7 +12,7 @@ import {Field, useFormState} from 'react-final-form';
 import {OnChange} from 'react-final-form-listeners';
 import {Button, Dropdown, Form, Input, Popup, Radio, TextArea, Icon} from 'semantic-ui-react';

-import {Checkbox} from 'indico/react/components';
+import Checkbox from 'indico/react/components/Checkbox';

 import formatters from './formatters';
 import parsers from './parsers';

foxbunny · 2024-09-09T11:10:43Z

Easy fix for the circular import :) (but don't ask me why this fixes it)

Nice. Damn, I thought I'd tried that already.

Speaking of those index modules that import bunch of stuff, I noticed that the mincss plugin also doesn't cope so well with them in SCSS. I don't know if we have automatic import ordering in SCSS, but we should, as apparently that helps.

ThiefMaster · 2024-09-09T14:08:02Z

indico/web/client/js/react/components/Checkbox.jsx

+
+import './Checkbox.module.scss';
+
+export default function Checkbox({label, onChange, ...inputProps}) {


One thing that's currently missing is support for the indeterminate state.

We don't use it with the FinalCheckbox but we have a few places where we use it with "standalone" checkboxes, for "select all/none" on top of tables (in that case the checkboxes typically don't have a visible label btw).

Ok, I'll add that.

I've also noticed this:

I feel that adding non-standard props would not be amazing for this, but I don't have a better idea short of creating a separate component (ToggleButton or something), which also doesn't sound amazing. 🤣

I've added support for the indeterminate state. It is passed in as a Boolean prop, separately from the checked prop, like you would with standard checkboxes. Separately, I've added a showAsToggle prop which makes it look like a toggle button (it's just cosmetics).

Do you want indeterminate state for the toggle button? It's just CSS, I can patch it in if needed.

tomasr8 · 2024-09-11T12:56:07Z

I'm being the bad guy again here, but I think it'd be better to keep the current checkbox style (icon, color) for consistency's sake, unless some changes are needed for a11y.

ThiefMaster · 2024-09-11T12:57:57Z

indico/web/client/js/react/forms/index.js

  FormFieldAdapter,
  unsortedArraysEqual,
-} from './fields';
+} = fields; // Fix circular dependency issue


I don't think this is still needed w/ the change on how we import the Checkbox component in fields.jsx?

I'll double-check in the morning.

foxbunny · 2024-09-11T15:55:56Z

'm being the bad guy again here, but I think it'd be better to keep the current checkbox style (icon, color)

Regarding the icon, replacing that would mean replacing all check marks everywhere. I'm happy to do that tho I find both checkmarks kinda ugly. :)) At least the old one is fatter so easier to spot and doesn't require a colored background to clearly see what's going on.

Regarding the color: reason as explained above. The checkmark is too thin.

tomasr8 · 2024-09-12T13:02:27Z

Regarding the icon, replacing that would mean replacing all check marks everywhere.

What do you mean by that? This PR only changes the SUI checkbox, right?

foxbunny · 2024-09-12T13:33:39Z

What do you mean by that? This PR only changes the SUI checkbox, right?

It doesn't. My understanding was that we won't vendor SUI and make modifications, so I reimplemented the checkbox. As such it uses the icomoon icon set.

foxbunny · 2024-09-30T05:52:42Z

I've changed the background color, but the icon question is still open. To be honest, the whole icon set looks a bit dated and might be better to do a complete rework at some point.

Short term we have the following options:

add a separate checkbox (so the set would have two)
replace the existing checkbox (applies to all places where checkboxes should be used)
do nothing (because it does not affect usability, and for accessibility purposes the whole checkbox is too small anyway, just like the default one)

As an FYI, from the accessibility perspective, the icon font as a whole must be replaced with an SVG spritesheet. This is not a high-priority fix as WCAG doesn't even mention it specifically, but it's a real problem for people with dyslexia who use custom fonts to make pages readable.

foxbunny · 2024-11-01T06:12:31Z

This is still blocking the error messages PR. Any chance we can get this moving?

Replace SUI Checkbox with a simpler component so that it can be programmatically focused on error.

ThiefMaster · 2024-11-01T11:23:55Z

indico/web/client/js/react/forms/fields.jsx

-} from 'semantic-ui-react';
+import {Button, Dropdown, Form, Input, Popup, Radio, TextArea, Icon} from 'semantic-ui-react';
+
+import Checkbox from 'indico/react/components/Checkbox';


Really weird, when I change this to import Checkbox from 'indico/react/components'; which should be pretty much equivalent since there's a re-report in components, I get this strange error/warning from webpack:

WARNING in chunk common [mini-css-extract-plugin] Conflicting order. Following module has been added: * css ../../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[2].oneOf[2].use[1]!../../../node_modules/resolve-url-loader/index.js??ruleSet[1].rules[2].oneOf[2].use[2]!../../../node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[2].oneOf[2].use[3]!../../../node_modules/sass-loader/dist/cjs.js??ruleSet[1].rules[2].oneOf[2].use[4]!./js/react/components/principals/items.module.scss despite it was not able to fulfill desired ordering with these modules: * css ../../../node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[2].oneOf[2].use[1]!../../../node_modules/resolve-url-loader/index.js??ruleSet[1].rules[2].oneOf[2].use[2]!../../../node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[2].oneOf[2].use[3]!../../../node_modules/sass-loader/dist/cjs.js??ruleSet[1].rules[2].oneOf[2].use[4]!./js/react/forms/fields.module.scss - couldn't fulfill desired order of chunk group(s) jquery, exports, module_events.abstracts, module_events.persons, module_events.registration, module_events.sessions, module_events.surveys, module_categories, module_categories.management, module_categories.calendar, module_events.display, module_events.header, module_receipts.admin, module_users.dashboard, module_users.profile_picture, module_users.favorites - while fulfilling desired order of chunk group(s) main, module_events.contributions, module_events.editing, module_events.management, module_events.papers, module_attachments, module_auth.signup, module_core, module_events.admin, module_events.roles, module_logs, module_rb, module_receipts, module_search, module_users.personal_data, module_users.data_export

(and a few more similar ones)

Do you have any idea which of your changes in this PR may be causing this? Because I have the feeling this may come back to haunt us in the future if some change is introducing an inconsistency w/ CSS ordering...

So while I don't have ideas as to what would cause this or how to find out what causes it, my hypothesis is that we are artificially enforcing an alphabetical order in the components whereas the actual order of (CSS) dependencies is (should be) different. To solve this issue once and for all we have two options:

Never import CSS in JSX modules

Never create god modules that import everything

I'm very much in favor of the second option as it also helps improve tree-shaking.

FinalCheckbox was changed to use the new Checkbox component in #6528, but many places still passed `toggle` instead of the new `showAsToggle` via the `FinalCheckbox` component to it. Also changed some places to use the new component (there are more though, but some of them require more style tweaks to not look weird).

FinalCheckbox was changed to use the new Checkbox component in indico#6528, but many places still passed `toggle` instead of the new `showAsToggle` via the `FinalCheckbox` component to it. Also changed some places to use the new component (there are more though, but some of them require more style tweaks to not look weird).

foxbunny force-pushed the checkbox-replacement branch 2 times, most recently from 9885779 to 0197dca Compare September 9, 2024 05:50

foxbunny force-pushed the checkbox-replacement branch from 0197dca to 016c7ed Compare September 9, 2024 07:31

ThiefMaster reviewed Sep 9, 2024

View reviewed changes

foxbunny force-pushed the checkbox-replacement branch from 016c7ed to 9e6100c Compare September 9, 2024 08:27

ThiefMaster reviewed Sep 9, 2024

View reviewed changes

foxbunny force-pushed the checkbox-replacement branch from 9e6100c to 414d8f8 Compare September 9, 2024 09:17

ThiefMaster reviewed Sep 9, 2024

View reviewed changes

foxbunny force-pushed the checkbox-replacement branch from d921920 to 7d9fc39 Compare September 10, 2024 06:58

ThiefMaster reviewed Sep 11, 2024

View reviewed changes

ThiefMaster added the a11y label Sep 11, 2024

foxbunny force-pushed the checkbox-replacement branch from b718f58 to 629510c Compare September 30, 2024 05:06

foxbunny force-pushed the checkbox-replacement branch from 629510c to 7e8193f Compare November 1, 2024 06:13

foxbunny and others added 7 commits November 1, 2024 12:00

Reimplement Checkbox

7c6ff1d

Replace SUI Checkbox with a simpler component so that it can be programmatically focused on error.

Fix test failure

3e6f71b

Update changelog

f055e1d

Clear default margin

0db47d3

Fix tests

08982ec

Add indeterminate state

490cb55

Add styling for indeterminate state and toggle mode

fd2bbab

foxbunny and others added 3 commits November 1, 2024 12:00

Remove fix for cicrular dep

da657f0

Remove background color from checkboxes

91b1977

Fix clickable label for privacy policy acknowledgement

787833c

ThiefMaster force-pushed the checkbox-replacement branch from 7e8193f to 787833c Compare November 1, 2024 11:15

ThiefMaster reviewed Nov 1, 2024

View reviewed changes

ThiefMaster added this to the v3.3 milestone Nov 4, 2024

ThiefMaster approved these changes Nov 4, 2024

View reviewed changes

ThiefMaster merged commit ea2a939 into indico:master Nov 4, 2024


		- Reimplement Checkbox to make it programmatically focusable (:pr:`6528`, thanks :user:`foxbunny`)

		- Nothing so far :(


		import './Checkbox.module.scss';

		export default function Checkbox({label, onChange, ...inputProps}) {

Reimplement Checkbox #6528

Reimplement Checkbox #6528

Uh oh!

Conversation

foxbunny commented Sep 9, 2024

Uh oh!

foxbunny commented Sep 9, 2024

Uh oh!

ThiefMaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThiefMaster commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foxbunny commented Sep 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomasr8 commented Sep 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foxbunny commented Sep 11, 2024

Uh oh!

tomasr8 commented Sep 12, 2024

Uh oh!

foxbunny commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foxbunny commented Sep 30, 2024

Uh oh!

foxbunny commented Nov 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ThiefMaster commented Sep 9, 2024 •

edited

Loading

foxbunny commented Sep 12, 2024 •

edited

Loading