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

feat(tailwind-components): Boolean field input #4630

Merged
merged 13 commits into from
Jan 29, 2025

Conversation

connoratrug
Copy link
Contributor

What are the main changes you did

  • explain what you changed and essential considerations.

How to test

  • explain here what to do to test this (or point to unit tests)

Checklist

  • updated docs in case of new feature
  • added/updated tests
  • added/updated testplan to include a test for this fix, including ref to bug using # notation

@connoratrug connoratrug changed the title feat: Boolean field input feat(tailwind-components): Boolean field input Jan 23, 2025
apps/tailwind-components/components/input/Boolean.vue Outdated Show resolved Hide resolved
}

const radioOptions = ref<RadioOptionsDataIF[]>([
{ value: true, label: "True" },
Copy link
Member

Choose a reason for hiding this comment

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

that you want to use this internally make a lot of sense to me.

const radioOptions = ref<RadioOptionsDataIF[]>([
{ value: true, label: "True" },
{ value: false, label: "False" },
]);
Copy link
Member

Choose a reason for hiding this comment

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

if not required then a 'clear' is null is possible?

@connoratrug connoratrug marked this pull request as ready for review January 23, 2025 14:29
@connoratrug connoratrug self-assigned this Jan 23, 2025
Copy link
Contributor

@chinook25 chinook25 left a comment

Choose a reason for hiding this comment

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

It works, but got a few small comments.

apps/tailwind-components/types/types.ts Outdated Show resolved Hide resolved
apps/tailwind-components/types/types.ts Outdated Show resolved Hide resolved
apps/tailwind-components/pages/input/Boolean.story.vue Outdated Show resolved Hide resolved
apps/tailwind-components/components/input/RadioGroup.vue Outdated Show resolved Hide resolved
apps/tailwind-components/components/input/Boolean.vue Outdated Show resolved Hide resolved
@connoratrug connoratrug requested a review from chinook25 January 27, 2025 15:48
Copy link
Contributor

@davidruvolo51 davidruvolo51 left a comment

Choose a reason for hiding this comment

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

Looks great. My only comment is about the prop align. Are there more alignment options other than vertical and horizontal. If not, maybe this could be a boolean prop. That would make it easier to use in other contexts.

:showClearButton="true"
align="horizontal"
@update:modelValue="onInput"
@focus="$emit('focus')"
Copy link
Contributor

Choose a reason for hiding this comment

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

(This is a fix for another PR)

When the radio group is focused, the inputs aren't visually focused. Focused styles for the input radio button is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what changes would you like to see ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this seems like an issue, on the master branch related to the radio input

Copy link
Contributor

@chinook25 chinook25 left a comment

Choose a reason for hiding this comment

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

Code looks fine now, however the when using the molgenis theme, the clear button is barely visible.

@connoratrug
Copy link
Contributor Author

Code looks fine now, however the when using the molgenis theme, the clear button is barely visible.

this is an effect of the demo page style , not element

@connoratrug connoratrug merged commit 01f7a24 into master Jan 29, 2025
7 checks passed
@connoratrug connoratrug deleted the feat/boolean-field-input branch January 29, 2025 10:46
davidruvolo51 pushed a commit that referenced this pull request Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants