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

Page duplicate dialog: don't require toggles #6922

Merged

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented Jan 18, 2025

Fixes #6921

Description

Summary of changes

  • Page duplicate dialog: don't require toggles

Reasoning

  • With required toggles, in v5 one cannot submit the form without activating the toggle, so false is no option.
  • Apparently, in v4 toggles could be on or off even when required.
  • I was wondering if we should change the general behavior for toggles (I assume also checkbox) to be the same as v4, but then required: true actually has no effect. So I do like that one can actually require a toggle/checkbox to be set. We just have to remove the required option from use cases like here where it's not right.

Changelog

Not from this PR, but so far missed...

Breaking changes

  • required: true on checkbox and toggle fields now enforces that these fields need to be checked/toggles (active state)

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add lab and/or sandbox examples (wherever helpful)
  • Add changes & docs to release notes draft in Notion

@distantnative distantnative added this to the 5.0.0-beta.2 milestone Jan 18, 2025
@distantnative distantnative requested a review from a team January 18, 2025 11:56
@distantnative distantnative self-assigned this Jan 18, 2025
@distantnative distantnative linked an issue Jan 18, 2025 that may be closed by this pull request
@afbora
Copy link
Member

afbora commented Jan 18, 2025

IMHO v5 behavior is correct. Otherwise required attribute would useless. But this is also a breaking change. If we continue with that we need to add to breaking changes in release notes.

Another way is deprecate this usage in v5 and remove it in v6.

Also PR is looks good to me and work great! 👍

@bastianallgeier
Copy link
Member

I agree that the new behavior makes a lot more sense. There's an indeterminate state for checkboxes: https://css-tricks.com/indeterminate-checkboxes/ but we don't support that yet at all. I think this would be the only way to have toggles and checkboxes that are not set yet. But with our current state, the required attribute does not make any sense in v4

@bastianallgeier bastianallgeier merged commit 6c9803e into v5/develop Jan 21, 2025
12 checks passed
@bastianallgeier bastianallgeier deleted the v5/fix/6921-page-duplicate-toggles-required branch January 21, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[v5] Duplicate action is forcing to copy files
3 participants