Slider and Range slider component

[kalashshah]

Pull Request Template

Ticket Number

• 1792

Description

Problem/Feature:

• Slider and Range slider components

Type of Change

• New feature

Checklist

• Quick PR: Is this a quick PR? Can be approved before finishing a coffee.
  • Quick PR label added
• Not Merge Ready: Is this PR dependent on some other PR/tasks and not ready to be merged right now.
  • DO NOT Merge PR label added

Frontend Guidelines

• Followed frontend guidelines as per the Guidelines Notion Doc

Build & Testing

• No errors in the build terminal
• Engineer has tested the changes on their local environment
• Engineer has tested the changes on deploy preview

Review & Approvals

• Self-review completed
• Code review by at least one other engineer
• Documentation updates if applicable

Notes

[github-actions]

• In the StyledSlider component, the CSS custom properties are being used but they are not defined within the provided code. It's assumed that these custom properties are defined elsewhere in the codebase.
• In the Slider component, there is a TypeScript error in the onChange function parameter declaration. The parameter names _ and value conflict with each other. Should be modified to something like (event, newValue).
• There is a duplicate import statement for strokeSemantics within the file.
• The sliderSemantics object is exporting the sliderSemantics from another place but being assigned to the same name. This can lead to confusion and should be named something else to avoid redundancy.

Overall, the code structure seems fine and the logic is clear.

Code Review Comments:

1. Define CSS custom properties used in StyledSlider.
2. Modify parameter names in Slider component's onChange function to avoid conflicts.
3. Remove duplicate import of strokeSemantics.
4. Rename sliderSemantics to avoid redundancy.

All looks good.

[github-actions]

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-09-11 08:30 UTC

[rohitmalhotra1420]

You need to mentioned both light and dark mode. This format is incorrect and will break.

[rohitmalhotra1420]

Sir we will be removing Material UI from our codebase in future, so this isn't gonna work. I guess we discussed this earlier.

We can only use those 3rd part libraries which offers standalone components like react and radix and not the whole design system like material UI.

[github-actions]

• In the Slider component, the parameter names in the onChange function should be unique. It's recommended to rename the second parameter since it conflicts with the MouseEvent type. Example: onChange={(_event: MouseEvent, sliderValue: T) => onChange(sliderValue)}
• In the semanticKeys object, toggle: 'components-toggle-switch' is not matching the actual semantic key 'components-switch'.
• The import statement for strokeSemantics and surfaceSemantics at the bottom is duplicated. You should remove the duplicate imports.

Apart from these issues, the code logic and structure seem fine.

Final Code:

import styled from 'styled-components';

import { Slider as MuiSlider } from '@material-ui/core';
import { SliderProps } from './Slider.types';

const StyledSlider = styled(MuiSlider)`
  .MuiSlider-rail {
    color: var(--components-slider-background-default);
    height: var(--spacing-xxxs);
    opacity: 1;
  }

  .MuiSlider-track {
    color: var(--components-slider-background-progress);
    height: var(--spacing-xxxs);
  }

  .MuiSlider-thumb {
    width: var(--spacing-sm);
    height: var(--spacing-sm);
    margin-top: -6px;
    background-color: var(--components-slider-icon-default);
    border: var(--border-sm) solid var(--components-slider-stroke-default);
  }

  .MuiSlider-thumb:hover,
  .MuiSlider-thumb:active,
  .MuiSlider-thumb:focus {
    box-shadow: none;
  }

  .MuiSlider-valueLabel {
    display: none;
  }
`;

const Slider = <T extends number | [number, number]>({
  min,
  max,
  onChange,
  value,
  step,
  defaultValue,
}: SliderProps<T>) => {
  return (
    <StyledSlider
      {...{ min, max, value, step, defaultValue }}
      onChange={(_event: MouseEvent, sliderValue: T) => onChange(sliderValue)}
    />
  );
};

Slider.displayName = 'Slider';

export { Slider };
export type SliderBaseProps = {
  min: number;
  max: number;
  step: number;
};

export type SliderProps<T> = SliderBaseProps & {
  value?: T;
  onChange: (value: T) => void;
  defaultValue?: T;
};

// semanticKeys object and colorSemantics object are unchanged from the previous code snippet

import { colorPrimitives } from '../colors/colors.primitives';
import { strokeSemantics } from './semantics.stroke';
import { surfaceSemantics } from './semantics.surface';

export const sliderSemantics = {
  'stroke-default': strokeSemantics.secondary,
  'icon-default': { light: colorPrimitives['white-100'], dark: colorPrimitives['white-100'] },
  'background-default': surfaceSemantics.secondary,
  'background-progress': surfaceSemantics['brand-medium'],
};

All looks good.

[github-actions]

File: package.json

• The version of "moment" package seems incorrect. It should be "2.30.1" instead of "2.30.1".

All looks good.

File: src/blocks/slider/Slider.tsx

• In the StyledSlider component, the CSS being applied in the else branch is incomplete. It seems like there is a missing closing curly brace '}'. It should be completed as follows:

: css`
    .horizontal-slider-track-0 {
        // Add styling here
    }
`

All looks good.

File: src/blocks/slider/Slider.types.ts

All looks good.

File: src/blocks/theme/colors/colors.semantics.ts

All looks good.

File: src/blocks/theme/semantics/semantics.slider.ts

All looks good.

File: yarn.lock

All looks good.

[github-actions]

In the package.json file:

1. The "vite-tsconfig-paths" dependency seems to be missing a comma at the end.
2. The "packageManager" key is at the wrong level in the file.

In the src/blocks/slider/Slider.tsx file:

1. There seems to be syntax issue in the StyledSlider component. The closing backtick and curly brace are not properly aligned.

All looks good.

[github-actions]

In the package.json file:

• The version of "moment" library seems incorrect. It should be "2.30.1" instead of "2.30.1".
• In the end of the file, there is an extra closing curly brace "}" that should be removed.
• The packageManager field should be defined in the root as an object like "packageManager": {"yarn": "4.2.2"}.

In the src/blocks/slider/Slider.tsx file:

• The CSS for the StyledSlider component is missing a closing curly brace after the .horizontal-slider-track-0 styling.
• There are unnecessary ellipses ("...") present in the file.

Everything else in the provided files looks good.

Corrected package.json:

...
"moment": "2.30.1",
...
"packageManager": {"yarn": "4.2.2"}
...

Corrected src/blocks/slider/Slider.tsx:

const StyledSlider = styled(ReactSlider)<{ range?: boolean }>`
  .horizontal-slider-track {
    height: var(--spacing-xxxs);
    background-color: var(--components-slider-background-default);
    border-radius: var(--border-sm);
  }
  .horizontal-slider-thumb {
    width: var(--spacing-sm);
    height: var(--spacing-sm);
    margin-top: -6px;
    background-color: var(--components-slider-icon-default);
    border: var(--border-sm) solid var(--components-slider-stroke-default);
    border-radius: 50%;
    cursor: pointer;
    &:active,
    &:focus {
      outline: none;
    }
  ${({ range }) =>
    range
      ? css`
          .horizontal-slider-track-1 {
            background-color: var(--components-slider-background-progress);
          }
        `
      : css`
          .horizontal-slider-track-0 {
          }
        `}
`;
const Slider = <T extends number | [number, number]>({
  min,
  max,
  onChange,
  value,
  step,
  defaultValue,
}: SliderProps<T>) => {
  return (
    <StyledSlider
      {...{ min, max, value, step, defaultValue }}
      className="horizontal-slider"
      thumbClassName="horizontal-slider-thumb"
      trackClassName="horizontal-slider-track"
      ariaLabel={isArray(value) ? ['Lower thumb', 'Upper thumb'] : 'Thumb'}
      ariaValuetext={(state) => `Thumb value ${state.valueNow}`}
      onChange={(value) => onChange(value as T)}
      value={value}
      range={isArray(value)}
    />
  );
};
Slider.displayName = 'Slider';
export { Slider };

All looks good.