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

Select field: remove undocumented empty prop #6459

Merged
merged 5 commits into from
May 28, 2024

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented May 25, 2024

This PR …

Revives #3373. Presented breaking case inside structure fields works by now.

By now the empty prop is redundant. Everything can be replicated by required, default and placeholder props.

Enhancements

  • default prop supports Kirby queries in radio and select field

Breaking changes

  • Select field: empty prop was removed. Use combination of required, placeholder and default to replicate functionality.

For review team

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

@distantnative distantnative added type: enhancement ✨ Suggests an enhancement; improves Kirby type: refactoring ♻️ Is about bad code; cleans up code labels May 25, 2024
@distantnative distantnative added this to the 5.0.0-alpha.1 milestone May 25, 2024
@distantnative distantnative requested a review from a team May 25, 2024 11:57
@distantnative distantnative self-assigned this May 25, 2024
@distantnative distantnative requested a review from a team May 25, 2024 11:57
Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

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

Beautifully simple. After wrapping my head around the logic, it makes a lot of sense to me.

panel/src/components/Forms/Input/SelectInput.vue Outdated Show resolved Hide resolved
Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

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

Gets my approval backend-wise 👍

@distantnative
Copy link
Member Author

distantnative commented May 25, 2024

@lukasbestle I spotted that it can be simplified even further: default in the Vue component had no real own functionality. It wouldn't actually set a default value. As other inputs, the default functionality depends on the backend writing the default to value.

So I removed the default prop from the Vue component and now the empty option is shown if the input is not required. Or if it's required, then it's only shown as long as the value is empty. Once it has a value, the empty option disappears.

Also allows us to get rid of the local selected state and just use value.

Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

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

I had written a comment asking about how the default prop works at all, but then realized the backend does it, so I answered my own question. But I didn't realize that required fields only need the empty state before they are first set. That's a good point and simultaneously clears up that confusion I had for the future.

TBH I don't fully get why we needed that selected attribute at all, but you know better why we don't need it anymore. If you are sure about the implementation, I think nothing speaks against merging the PR, otherwise someone else should review the frontend code.

@distantnative
Copy link
Member Author

@afbora would be great to get your review and tests here as well. Always good to be double safe with such changes.

Copy link
Member

@afbora afbora left a comment

Choose a reason for hiding this comment

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

LGTM and works great 👍

@distantnative distantnative merged commit 6355252 into v5/develop May 28, 2024
7 checks passed
@distantnative distantnative deleted the v5/fix/select-empty branch May 28, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby type: refactoring ♻️ Is about bad code; cleans up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants