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): add UiToggle component #8084

Merged
merged 3 commits into from
Nov 15, 2024
Merged

feat(web-core): add UiToggle component #8084

merged 3 commits into from
Nov 15, 2024

Conversation

J0ris-K
Copy link
Contributor

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

Description

add UiToggle component

Screenshot

image
image
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 30, 2024
@J0ris-K J0ris-K force-pushed the xo6/toggle branch 3 times, most recently from e4a397d to 325b5c3 Compare November 5, 2024 07:57
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.

There are small mismatches between this component and the DS.

The toggle circle seems "inside" its container, where on the DS it should be "on top" of it (we shouldn't differentiate the circle border from the container border)

There should be a gap around the spinner icon.

The border of the circle border doesn't seem to be animated, and this creates a time lag between it and the container border animation.

BTW, I think the speed is a little bit too fast. For this kind of interaction, I'd use a 250 ms transition instead.

Finally, the code is maybe a little too complex with 6-level depth.

I'd propose a different approach that:

  • doesn't change UiSpinner
  • animates all the borders and the spinner icon opacity with the same animation timing
  • uses a CSS variable for borders since we always want the same color to be used for all of them.
  • reduces the complexity and code depth
  • uses better class names
  • rename toggleModel to checked
  • add the gap around the spinner
<!-- v2 -->
<template>
  <label class="ui-toggle typo c2-semi-bold">
    <slot />
    <span class="toggle-container">
      <input v-model="checked" :disabled="isDisabled || busy" class="input" type="checkbox" />
      <span :class="{ checked }" class="toggle-icon">
        <UiSpinner :class="{ visible: busy }" class="spinner" />
      </span>
    </span>
  </label>
</template>

<script lang="ts" setup>
import UiSpinner from '@core/components/UiSpinner.vue'
import { useContext } from '@core/composables/context.composable'
import { DisabledContext } from '@core/context'

const props = withDefaults(
  defineProps<{
    disabled?: boolean
    busy?: boolean
  }>(),
  { disabled: undefined }
)

const checked = defineModel<boolean>()

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

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

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

  .toggle-container {
    --transition-timing: 0.25s ease-in-out;
    --border-color: var(--color-neutral-txt-secondary);
    height: 2rem;
    width: 4rem;
    background-color: var(--color-neutral-background-primary);
    border: 0.1rem solid var(--border-color);
    border-radius: 1rem;
    transition:
      background-color var(--transition-timing),
      border-color var(--transition-timing);

    &:has(input:disabled) {
      --border-color: var(--color-neutral-border);
      background-color: var(--color-neutral-background-disabled);
      cursor: not-allowed;
    }

    &:has(input:checked) {
      background-color: var(--color-success-item-base);
    }

    &:has(input:checked:disabled) {
      background-color: var(--color-success-item-disabled);
    }

    &:has(input:focus-visible) {
      outline: none;

      &::after {
        position: absolute;
        content: '';
        inset: -0.5rem;
        border: 0.2rem solid var(--color-normal-txt-base);
        border-radius: 0.4rem;
      }
    }
  }

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

  .toggle-icon {
    display: flex;
    align-items: center;
    justify-content: center;
    font-size: 1.4rem;
    width: 2rem;
    height: 2rem;
    margin: -0.1rem 0 0 -0.1rem;
    color: var(--color-normal-txt-base);
    background-color: var(--color-neutral-background-primary);
    border: 0.1rem solid var(--border-color);
    border-radius: 1rem;
    transition:
      transform var(--transition-timing),
      border-color var(--transition-timing);

    &.checked {
      transform: translateX(2rem);
    }
  }

  .spinner {
    opacity: 0;
    transition: opacity var(--transition-timing);

    &.visible {
      opacity: 1;
    }
  }
}
</style>

@xen-orchestra/web-core/lib/components/UiSpinner.vue Outdated Show resolved Hide resolved
@J0ris-K
Copy link
Contributor Author

J0ris-K commented Nov 12, 2024

Nice proposal and thanks !

@OlivierFL OlivierFL merged commit 36bab99 into master Nov 15, 2024
1 check passed
@OlivierFL OlivierFL deleted the xo6/toggle branch November 15, 2024 08:03
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.

3 participants