indico icon indicating copy to clipboard operation
indico copied to clipboard

Reimplement Checkbox

Open foxbunny opened this issue 1 year ago • 8 comments

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

foxbunny avatar Sep 09 '24 04:09 foxbunny

@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.

foxbunny avatar Sep 09 '24 07:09 foxbunny

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';

ThiefMaster avatar Sep 09 '24 11:09 ThiefMaster

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.

foxbunny avatar Sep 09 '24 11:09 foxbunny

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. image(1) image(2)

tomasr8 avatar Sep 11 '24 12:09 tomasr8

'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.

foxbunny avatar Sep 11 '24 15:09 foxbunny

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?

tomasr8 avatar Sep 12 '24 13:09 tomasr8

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 avatar Sep 12 '24 13:09 foxbunny

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 avatar Sep 30 '24 05:09 foxbunny

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

foxbunny avatar Nov 01 '24 06:11 foxbunny