Replace the SUI radio button in registration form #6621

foxbunny · 2024-11-20T03:24:49Z

Implement a RadioButton component to replace the SUI radio button
Replace usages of SUI radio button within the reigstration form inputs
Address the issue of SingleChoiceInput needing both fieldset and non-fieldset rendering depending on the settings by modifying the renderAsFieldset option to allow a function that returns the correct value based on the settings

indico/modules/events/registration/client/js/form/FormItem.jsx

ThiefMaster · 2024-11-20T11:49:51Z

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

+                <RadioButton
                  id={id ? `${id}-${index}` : ''}
-                  style={{pointerEvents: 'auto'}} // keep label tooltips working when disabled
-                  styleName="radio"


the .radio rule can now be removed from table.module.scss since it's not used anywhere else

ThiefMaster · 2024-11-20T11:53:51Z

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

-                <Form.Radio
+                <RadioButton
                  id={id ? `${id}-${index}` : ''}
-                  style={{pointerEvents: 'auto'}} // keep label tooltips working when disabled


This was there for a reason: When you have a registration that allows modifications even after payment (typically "until modification deadline"), and changing the field would result in a price change on an already-paid registration, we show this indicator. By removing this, this tooltip on the ⚠ sign no longer shows up:

This change fixes the behavior:

diff --git a/indico/web/client/js/react/components/RadioButton.module.scss b/indico/web/client/js/react/components/RadioButton.module.scss index 1564d31aca..39a889b946 100644 --- a/indico/web/client/js/react/components/RadioButton.module.scss +++ b/indico/web/client/js/react/components/RadioButton.module.scss @@ -11,6 +11,7 @@ display: inline-flex; gap: var(--content-gap-normal); align-items: center; + pointer-events: auto; } .radio {

It should be a toggletip not a tooltip.

It's illegal for a label to contain multiple interactive elements for obvious reasons.

Therefore I'll render the toggletip separately, outside the label, and this hack won't be needed.

OK, will you add it to this PR?

PS: The hack might be needed nonetheless, just on another element, since the SUI Field applies pointer-events: none; for disabled fields. But let's see ;)

I swear "semantic" is a misnomer. :) Anyway, I'll add it to this PR. If I end up hacking something at least I'll be able to move it to the CSS where it belongs.

Looks like I'll be doing this in a separate PR after all. I've cleaned up the CSS a bit to get this to work in a more generic way that covers all fieldsets.

I'll do a separate PR for the toggletip bit.

OK, but I can't merge this PR until the other fix is ready as well (especially now that the icon itself is also gone)

What do you mean by "icon is gone"? Can you give me a screenshot?

label={<ChoiceLabel choice={c} management={management} paid={isPaidChoice(c)} />}

You removed this line and replaced it with the plain label={c.caption}. The ChoiceLabel component is also responsible for showing the warning icon when the field is locked.

Hmm, I thought I reverted that, but it looks like the build just crashed. My bad.

CHANGES.rst

tomasr8 · 2024-11-20T15:42:46Z

Thanks for the PR!

I don't know if it's just my browser/screen or w/e but the radio does not always look circular:

Compare with the current radio:

It's also a bit larger now, personally I think I prefer the current smaller size

- Implement a RadioButton component to replace the SUI radio button - Replace usages of SUI radio button within the reigstration form inputs - Address the issue of SingleChoiceInput needing both fieldset and non-fieldset rendering depending on the settings by modifying the renderAsFieldset option to allow a function that returns the correct value based on the settings - Remove redundant .radio rule

- Fix SUI styles for disabled controls affecting fieldsets - Add disabled attribute to fieldsets so they can be targeted in that state - Add a name attribute to radio buttons so they behave properly as a group

foxbunny · 2024-11-21T05:51:41Z

I don't know if it's just my browser/screen or w/e but the radio does not always look circular:

I wasn't able to reproduce this, but I added flex: none to it. Let me know if that fixes it.

It's also a bit larger now, personally I think I prefer the current smaller size

The idea is that it roughly matches the checkbox size. It's still a bit small as far as recommended click target size goes, and normally I would add some padding around it to compensate, but it would only work if we could make the font sizes a bit larger overall (at least 100% of the default size) which we currently can't. So this is a compromise size.

tomasr8 · 2024-11-21T09:58:46Z

The idea is that it roughly matches the checkbox size. It's still a bit small as far as recommended click target size goes, and normally I would add some padding around it to compensate, but it would only work if we could make the font sizes a bit larger overall (at least 100% of the default size) which we currently can't. So this is a compromise size.

Fair enough 👍

I wasn't able to reproduce this, but I added flex: none to it. Let me know if that fixes it.

It still happens and I don't really know why but it seems to have something to do with fractional widths. 1.2em ends up being 16.8px which looks off to me. However both 16px and 17px look fine. Here's a comparison between 16.8px and 17px. It's pretty subtle but 16.8px looks like it has sharp corner on the main axes:

foxbunny · 2024-11-21T10:17:01Z

To me both looks a bit off and pixelated, but that's probably a combination of how your browser renders it, the compression of the image, etc. At any rate, I don't do pixel values and there's a good reason for that. There's currently no advantage in using em/rem over PX because the base stylesheet is broken, but once we get around to fixing that, it will make all the difference in the world. I wouldn't make exceptions because some browsers render poorly at the sub pixel level.

foxbunny · 2024-11-26T03:15:19Z

@ThiefMaster Do you want me to do anything else with this PR? I'm sort of needing it for the error messages PR and also to take care of the toggle tip one. The first one is blocked by this, and the second one will be easier to do once this is merged.

tomasr8 · 2024-11-26T10:41:50Z

Is there any chance we could avoid fractional sizes like SUI does? I checked on Chrome as well, and the black dot looks a bit off sometimes:

foxbunny · 2024-11-26T10:46:47Z

The real question is do we want to give up hope of having a fully responsive layout (which is an a11y requirement) at some point in future because this doesn't render correctly on some machines. What's the priority. As far as I'm aware you can't have it both ways. You're either always using relative units or you're not. Can't mix and match, and can't have full responsiveness with pixel units.

tomasr8 · 2024-11-26T15:17:23Z

Is there any way we could use something like round() to get a more consistent look? Otherwise I agree with you that responsiveness is more important, it just feels a bit jarring when compared with the current radio..

foxbunny · 2024-11-26T15:21:37Z

The optimal solution is to simply give it more pixels to work with to begin with. However, that means larger text as well, and that means fully responsive layout is a must. That's still a long way out.

I haven't heard about any round() that would round to the nearest pixel. In a layout that uses all relative units, that would probably be a bit too unpredictable to be useful.

tomasr8 · 2024-11-26T15:55:28Z

I haven't heard about any round() that would round to the nearest pixel. In a layout that uses all relative units, that would probably be a bit too unpredictable to be useful.

round looks like it can do it but I just threw it out there as a possibility, I've never used it myself. If you think it's not possible to get around those sub-pixel rendering issues, I trust your judgement :)

foxbunny · 2024-11-26T16:00:53Z

round looks like it can do it

It only rounds the value, and doesn't perform unit conversion (afaik), so it's not quite the same. What you're looking for is something like pixel-align: nearest (I made it up, ofc), but it could actually result in off-by-one-pixel issues, where one element is rounded up, and another down, so I don't think it's even possible.

tomasr8 · 2024-11-27T09:11:01Z

Is there anything you still need to do in this PR? Otherwise, I think it's looking good :)

foxbunny · 2024-11-27T09:27:54Z

No, I'm good

- Implement a RadioButton component to replace the SUI radio button - Replace usages of SUI radio button within the registration form inputs - Address the issue of SingleChoiceInput needing both fieldset and non-fieldset rendering depending on the settings by modifying the renderAsFieldset option to allow a function that returns the correct value based on the settings - Remove redundant .radio rule - Fix SUI styles for disabled controls affecting fieldsets - Add disabled attribute to fieldsets so they can be targeted in that state - Add a name attribute to radio buttons so they behave properly as a group

foxbunny force-pushed the radio branch from 1ee7c2d to 7ef9256 Compare November 20, 2024 03:25

foxbunny requested review from ThiefMaster and tomasr8 November 20, 2024 03:25

ThiefMaster reviewed Nov 20, 2024

View reviewed changes

indico/modules/events/registration/client/js/form/FormItem.jsx Show resolved Hide resolved

ThiefMaster reviewed Nov 20, 2024

View reviewed changes

ThiefMaster added the a11y label Nov 20, 2024

foxbunny added 2 commits November 21, 2024 07:19

Fix SUI issues and radio focus

0cec27c

- Fix SUI styles for disabled controls affecting fieldsets - Add disabled attribute to fieldsets so they can be targeted in that state - Add a name attribute to radio buttons so they behave properly as a group

foxbunny force-pushed the radio branch from 1f0878f to 0cec27c Compare November 21, 2024 05:49

foxbunny added 2 commits November 21, 2024 14:24

Add PR number

af93023

Revert single choice input label

addb53c

ThiefMaster approved these changes Nov 28, 2024

View reviewed changes

ThiefMaster merged commit 4c5f932 into indico:master Nov 28, 2024
9 checks passed

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

Replace the SUI radio button in registration form #6621

Replace the SUI radio button in registration form #6621

Uh oh!

Conversation

foxbunny commented Nov 20, 2024

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!

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!

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!

Uh oh!

Uh oh!

tomasr8 commented Nov 20, 2024

Uh oh!

foxbunny commented Nov 21, 2024

Uh oh!

tomasr8 commented Nov 21, 2024

Uh oh!

foxbunny commented Nov 21, 2024

Uh oh!

foxbunny commented Nov 26, 2024

Uh oh!

tomasr8 commented Nov 26, 2024

Uh oh!

foxbunny commented Nov 26, 2024

Uh oh!

tomasr8 commented Nov 26, 2024

Uh oh!

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

Uh oh!

Uh oh!

tomasr8 commented Nov 26, 2024

Uh oh!

foxbunny commented Nov 26, 2024

Uh oh!

tomasr8 commented Nov 27, 2024

Uh oh!

foxbunny commented Nov 27, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

foxbunny commented Nov 26, 2024 •

edited

Loading