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(web-core): added UiRadioButton component #8067

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

J0ris-K
Copy link
Contributor

@J0ris-K J0ris-K commented Oct 23, 2024

Description

Added UiRadioButton component

Screeshot

image

Checklist

  • Commit
    • Title follows commit conventions
    • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
    • If bug fix, add Introduced by
  • Changelog
    • If visible by XOA users, add changelog entry
    • Update "Packages to release" in CHANGELOG.unreleased.md
  • PR
    • If UI changes, add screenshots
    • If not finished or not tested, open as Draft

Review process

This 2-passes review process aims to:

  • develop skills of junior reviewers
  • limit the workload for senior reviewers
  • limit the number of unnecessary changes by the author
  1. The author creates a PR.
  2. Review process:
    1. The author assigns the junior reviewer.
    2. The junior reviewer conducts their review:
      • Resolves their comments if they are addressed.
      • Adds comments if necessary or approves the PR.
    3. The junior reviewer assigns the senior reviewer.
    4. The senior reviewer conducts their review:
      • If there are no unresolved comments on the PR → merge.
      • Otherwise, we continue with 3.
  3. The author responds to comments and/or makes corrections, and we go back to 2.

Notes:

  1. The author can request a review at any time, even if the PR is still a Draft.
  2. In theory, there should not be more than one reviewer at a time.
  3. The author should not make any changes:
    • When a reviewer is assigned.
    • Between the junior and senior reviews.

@J0ris-K J0ris-K self-assigned this Oct 23, 2024
@J0ris-K J0ris-K force-pushed the xo6/radio-button branch 6 times, most recently from c36f179 to 4a4bfa5 Compare October 29, 2024 10:15
@J0ris-K J0ris-K force-pushed the xo6/radio-button branch 8 times, most recently from 72a1f84 to 41482b8 Compare November 6, 2024 15:49
Copy link
Contributor

@ByScripts ByScripts left a comment

Choose a reason for hiding this comment

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

I think the code can be simplified a lot here.

Also, the icon seems misaligned (it should look like the bottom version):

radio button

Finally, I don't think the info slot should be part of the Radio component. When looking at the DS, it seems that it rather belongs to the Radio Group component.

Here is a suggestion:

<template>
  <label class="ui-radio-button typo p1-regular">
    <span :class="variant" class="radio-container">
      <input v-model="model" :disabled="isDisabled" :value class="input" type="radio" />
      <VtsIcon :icon="faCircle" accent="current" class="radio-icon" />
    </span>
    <slot />
  </label>
</template>

<script lang="ts" setup>
import VtsIcon from '@core/components/icon/VtsIcon.vue'
import { useContext } from '@core/composables/context.composable'
import { DisabledContext } from '@core/context'
import { toVariants } from '@core/utils/to-variants.util'
import { faCircle } from '@fortawesome/free-solid-svg-icons'
import { computed } from 'vue'

const props = withDefaults(
  defineProps<{
    accent: 'info' | 'success' | 'warning' | 'danger'
    value: any
    disabled?: boolean
  }>(),
  {
    disabled: undefined,
  }
)

const model = defineModel<boolean>()

defineSlots<{
  default(): any
}>()

const variant = computed(() => toVariants({ accent: props.accent }))

const isDisabled = useContext(DisabledContext, () => props.disabled)
</script>

<style lang="postcss" scoped>
.ui-radio-button {
  display: inline-flex;
  align-items: center;
  gap: 0.8rem;

  .radio-container {
    display: inline-flex;
    align-items: center;
    justify-content: center;
    border: 0.2rem solid var(--accent-color);
    background-color: transparent;
    border-radius: 0.8rem;
    width: 1.6rem;
    height: 1.6rem;
    transition:
      border-color 0.125s ease-in-out,
      background-color 0.125s ease-in-out;

    &:has(.input:disabled) {
      cursor: not-allowed;

      /*
      TODO: To be removed or kept after a decision has been taken
      See https://www.figma.com/design/l2O2VvzJRnOCvqxhM7d124?node-id=8-1940#1021964394
      */

      &:not(:has(.input:checked)) {
        --accent-color: var(--color-neutral-txt-secondary);
      }
    }

    &:has(.input:checked) {
      background-color: var(--accent-color);
    }

    &.accent--info {
      --accent-color: var(--color-info-item-base);

      &:hover {
        --accent-color: var(--color-info-item-hover);
      }

      &:active {
        --accent-color: var(--color-info-item-active);
      }

      &:has(.input:disabled) {
        --accent-color: var(--color-info-item-disabled);
      }
    }

    &.accent--success {
      --accent-color: var(--color-success-item-base);

      &:hover {
        --accent-color: var(--color-success-item-hover);
      }

      &:active {
        --accent-color: var(--color-success-item-active);
      }

      &:has(.input:disabled) {
        --accent-color: var(--color-success-item-disabled);
      }
    }

    &.accent--warning {
      --accent-color: var(--color-warning-item-base);

      &:hover {
        --accent-color: var(--color-warning-item-hover);
      }

      &:active {
        --accent-color: var(--color-warning-item-active);
      }

      &:has(.input:disabled) {
        --accent-color: var(--color-warning-item-disabled);
      }
    }

    &.accent--danger {
      --accent-color: var(--color-danger-item-base);

      &:hover {
        --accent-color: var(--color-danger-item-hover);
      }

      &:active {
        --accent-color: var(--color-danger-item-active);
      }

      &:has(.input:disabled) {
        --accent-color: var(--color-danger-item-disabled);
      }
    }

    /* INPUT */

    .input {
      opacity: 0;
      position: absolute;
      pointer-events: none;
    }

    /* ICON */

    .radio-icon {
      font-size: 0.8rem;
      position: absolute;
      opacity: 0;
      transition: opacity 0.125s ease-in-out;
      color: var(--color-neutral-txt-primary);
    }

    &.accent--danger .radio-icon {
      color: var(--color-danger-txt-item);
    }

    .input:disabled + .radio-icon {
      color: var(--color-neutral-txt-secondary);
    }

    .input:checked + .radio-icon {
      opacity: 1;
    }
  }
}
</style>

@J0ris-K
Copy link
Contributor Author

J0ris-K commented Nov 12, 2024

@ByScripts Are you sure for the info slot ? because if you look there it appears to be for both components (even though there is a checkbox that appears to be a copy and paste of the checkbox component). Maybe we can ask to @clemencebx ?
image

@ByScripts
Copy link
Contributor

ByScripts commented Nov 12, 2024

@ByScripts Are you sure for the info slot ? because if you look there it appears to be for both components (even though there is a checkbox that appears to be a copy and paste of the checkbox component). Maybe we can ask to @clemencebx ?

Yes, I think we should wait for @clemencebx because I don't think it would make sense to have an individual message for each input 🤔

IMO, in case we want to add a specific "help" for an input, it should probably be through an "Info" icon next to the label.

Edit: Link to Figma comment

@J0ris-K
Copy link
Contributor Author

J0ris-K commented Nov 12, 2024

Yes, I agree with you !

@J0ris-K
Copy link
Contributor Author

J0ris-K commented Nov 12, 2024

image
@ByScripts I haven't changed anything yet, but I don't see any problem with the centering of the icon?

@clemencebx
Copy link

@ByScripts Are you sure for the info slot ? because if you look there it appears to be for both components (even though there is a checkbox that appears to be a copy and paste of the checkbox component). Maybe we can ask to @clemencebx ?

Yes, I think we should wait for @clemencebx because I don't think it would make sense to have an individual message for each input 🤔

IMO, in case we want to add a specific "help" for an input, it should probably be through an "Info" icon next to the label.

Edit: Link to Figma comment

Checkbox + Label:

  • A single checkbox item, like “Remember me” on a login page or “I accept Terms and Conditions” in a form, has its info slot directly linked to the individual item.
  • When there are multiple, standalone checkboxes, like a list of optional advanced settings, each item can have its own info slot for additional context.
  • For grouped checkboxes, such as when you need to select one or more options from a list in a form, they should always have a label for the group, and the info slot applies to the entire group, providing context for the collection of options.

Radiobox + Label:

  • Radio buttons are always presented as a group, allowing a single choice within a list. They always should have a label for the group, and the info slot applies to the entire group rather than individual items.

Does this structure work for you? If so, I’ll update the Figma documentation accordingly to reduce any confusion.

@J0ris-K J0ris-K force-pushed the xo6/radio-button branch 2 times, most recently from 27511c5 to 789efe4 Compare November 13, 2024 10:54
Comment on lines 180 to 193
.radio-container .radio-icon {
color: var(--color-danger-txt-item);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this selector can be simplified to:

Suggested change
.radio-container .radio-icon {
color: var(--color-danger-txt-item);
}
.radio-icon {
color: var(--color-danger-txt-item);
}

And it should be used in the other accent variants with the right values, since the icon color also changes for info, success and warning. Here is a screenshot of the design system; we can see the value is --color-info-txt-item for the info variant:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants