Components/progress bar #1756

NAsriGhada · 2024-04-02T21:15:08Z

I'm facing an error when using Radix UI components in our Tailwind-configured project. After migrating from JSX I got a different error, attached in the screenshot, which suggests a type incompatibility with the Progress components as far as I understood.

Inline styles were a successful interim solution for the initial Tailwind conflict, but this TypeScript issue persists.

changeset-bot · 2024-04-02T21:15:11Z

⚠️ No Changeset found

Latest commit: fc4570d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Enjoy2Live · 2024-04-02T21:20:09Z

@NAsriGhada type compatibility is due to misaligned @types/react deps coming from radix-ui so it's not a big problem and we will fix it in the future.

as for using css classnames rather than tailwind, why not use tailwind utility classes directly?

Enjoy2Live · 2024-04-02T21:21:29Z

packages/UI/src/components/progressBar/styles.css

@@ -0,0 +1,21 @@
+@import '@radix-ui/colors/black-alpha.css';


why importing radix colors? we only use the colors provided by our custom tailwind config

yeah just for testing, I will fix it.

Enjoy2Live

pretty good start, just a little bit changes here and there and it's ready to be merged!

please go over my requested changes and let me know here if you have updates or questions.

Enjoy2Live · 2024-04-08T04:24:03Z

packages/UI/src/components/progressBar/ProgressBar.tsx

@@ -0,0 +1,66 @@
+import * as Progress from '@radix-ui/react-progress';
+import React from 'react';
+import { tv } from 'tailwind-variants';


why importing tv but never using it? I suggest removing this import

Enjoy2Live · 2024-04-08T04:28:10Z

packages/UI/src/components/progressBar/ProgressBar.tsx

+        style={{
+          width,
+          height,
+          backgroundColor,
+        }}


Why not doing these in tailwind? you can do width based on percentage using tailwind here's an example of how it's done

backgroundColor can be used in this way bg-{colorToken}

height can be written this way h-full which translates to 100%

Yeah I tried that, but since the colors are dynamic in the progress bar it didn't work as I expected it to be, but I'll try to find a solution.

Yeah I tried that, but since the colors are dynamic in the progress bar it didn't work as I expected it to be, but I'll try to find a solution.

Read this and you'll have an idea of how to do it
https://tailwindcss.com/docs/content-configuration#dynamic-class-names

Enjoy2Live · 2024-04-08T04:29:14Z

packages/UI/src/components/progressBar/ProgressBar.tsx

+        value={progress}
+      >
+        <Progress.Indicator
+          style={indicatorStyle}


Why not doing these in tailwind? you can do width based on percentage using tailwind here's an example of how it's done

backgroundColor can be used in this way bg-{colorToken}

height can be written this way h-full which translates to 100%

packages/UI/src/components/progressBar/ProgressBar.tsx

Enjoy2Live · 2024-04-08T04:36:37Z

packages/UI/src/components/progressBar/ProgressBar.tsx

+interface ProgressProps {
+  startColor?: string;
+  endColor?: string;
+  backgroundColor?: string;
+  width?: string;
+  height?: string;
+}


I'd recommend you take the component structure from shadcn.

This will ensure you follow the best practices and customize the component however you see fit.

Enjoy2Live · 2024-04-08T04:38:55Z

packages/UI/src/components/progressBar/ProgressBar.tsx

+    width: `${progress}%`,
+  };
+
+  // const progressClass = progress === 100 ? endColor : startColor;


this commented line should be deleted

Enjoy2Live · 2024-04-08T04:43:40Z

packages/UI/src/components/progressBar/ProgressBar.tsx

+          className="transition-width duration-500 ease-in-out"
+        />
+      </Progress.Root>
+      <p>Progress: {progress}%</p>


This should be <span id="loadinglabel"> tag rather than paragraph.

and the Progress.Root should have this prop
aria-labelledby="loadinglabel"

taken from mdn

Enjoy2Live · 2024-04-08T05:02:58Z

packages/UI/src/components/progressBar/ProgressBar.stories.tsx

+    startColor: {
+      control: 'color',
+      description: 'The starting color of the progress bar.',
+    },
+    endColor: {
+      control: 'color',
+      description: 'The color of the progress bar when progress is complete.',
+    },
+    width: {
+      control: 'text',
+      description: 'The width of the progress bar.',
+    },
+    height: {
+      control: 'text',
+      description: 'The height of the progress bar.',
+    },
+    backgroundColor: {
+      control: 'color',
+      description: 'The background color of the progress bar container.',
+    },


These are auto generated by storybook and you don't have to write them in .stories files.

To have a description of each prop try adding jsdoc to each typescript prop definition.

example from packages/UI/src/components/Checkbox/Checkbox.tsx

/** * When true, prevents the user from interacting with the checkbox. */ disabled?: RadixCheckbox.CheckboxProps['disabled']; /** * The controlled checked state of the checkbox. Must be used in conjunction with onCheckedChange. */ checked?: RadixCheckbox.CheckedState & NonNullable<unknown>; /** * The checked state of the checkbox when it is initially rendered. Use when you do not need to control its checked state. */ defaultChecked?: RadixCheckbox.CheckedState & NonNullable<unknown>;

Enjoy2Live · 2024-04-08T05:05:26Z

packages/UI/src/components/progressBar/ProgressBar.stories.tsx

+import ProgressBar from './ProgressBar';
+
+const meta: Meta<typeof ProgressBar> = {
+  title: 'Components/ProgressBar',


no need for this, omitting the title makes storybook read the path of .stories file and place it in here.

Enjoy2Live · 2024-04-08T05:05:47Z

packages/UI/src/components/progressBar/ProgressBar.stories.tsx

+const meta: Meta<typeof ProgressBar> = {
+  title: 'Components/ProgressBar',
+  component: ProgressBar,
+  tags: ['autodocs'],


no need for this as docs are always true in our config

Enjoy2Live · 2024-04-17T08:03:14Z

packages/UI/src/components/progressBar/ProgressBar.tsx

+  startColor,
+  endColor,
+  backgroundColor,
+  width,
+  height,


these props aren't needed as they're all going to be fixed

Ok perfect!

Enjoy2Live · 2024-04-20T11:02:07Z

packages/UI/src/components/progressBar/ProgressBar.stories.tsx

+    theme: {
+      control: {
+        type: 'select',
+        options: 'light',
+      },


when did we decide to include this theme prop?

Enjoy2Live · 2024-04-20T11:02:14Z

packages/UI/src/components/progressBar/ProgressBar.stories.tsx

-    backgroundColor: 'bg-gray-300',
-    width: 'w-80',
-    height: 'h-6',
+    theme: 'light',


when did we decide to include this theme prop?

Enjoy2Live · 2024-04-20T11:02:22Z

packages/UI/src/components/progressBar/ProgressBar.tsx

+  /**
+   * the theme prop controls the appearance based on predefined styles in a themes object.
+   */
+  theme: 'light';


when did we decide to include this theme prop?

Enjoy2Live · 2024-04-20T11:02:29Z

packages/UI/src/components/progressBar/ProgressBar.tsx

  progress,
+  theme = 'light',


when did we decide to include this theme prop?

Enjoy2Live · 2024-04-20T11:02:44Z

packages/UI/src/components/progressBar/ProgressBar.tsx

-  backgroundColor,
-  width,
-  height,
+  // backgroundColor,


why commenting backgroundColor instead of deleting?

…blity see: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/progressbar_role#examples

Enjoy2Live

LOOKS AMAZING, you've really nailed this one

I left a tiny request change that can totally be overlooked and a PR that adds to this one here with each commit explaining why it the change was made:
#1788

Please go over the linked PR and let me know if you have questions and if not merge the linked PR then merge this yours after.

thanks a ton Ghada!

Enjoy2Live · 2024-05-05T14:20:46Z

packages/UI/src/components/progressBar/Progress.tsx

+        className={`${styles.flex} ${styles.flexDirection} ${styles.alignItems} gap-14 rounded-lg p-4`}
+      >
+        <div
+          className={`${styles.flex} ${styles.width} ${styles.flexDirection} ${styles.alignItems} gap-3`}


I think styles.flex,styles.width, styles.flexDirection , styles.alignItems can be used directly instead

ls there a reason why you made it in an object?

Yeah simply for code readability and the DRY principle which made the lines shorter.

progressbar changes

dev-launchers-flux · 2024-06-16T04:03:21Z

🎉 This PR is included in version 2.17.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

resolve progressBar issues

066f2f3

NAsriGhada requested a review from a team as a code owner April 2, 2024 21:15

Enjoy2Live reviewed Apr 2, 2024

View reviewed changes

Enjoy2Live marked this pull request as draft April 2, 2024 21:46

progress bar preview

6fa4944

NAsriGhada closed this Apr 4, 2024

NAsriGhada deleted the components/progress-bar branch April 4, 2024 19:45

NAsriGhada added 2 commits April 4, 2024 21:16

progreesbar

9f1502b

progress bar with storybook

5f8187f

Enjoy2Live reviewed Apr 8, 2024

View reviewed changes

Enjoy2Live reopened this Apr 8, 2024

Enjoy2Live changed the title ~~resolve progressBar issues~~ Components/progress bar Apr 8, 2024

NAsriGhada added 2 commits April 11, 2024 19:44

description not working

1c9627b

description not working and not autogenerated

10e5522

NAsriGhada marked this pull request as ready for review April 13, 2024 15:12

Enjoy2Live requested changes Apr 17, 2024

View reviewed changes

NAsriGhada added 2 commits April 18, 2024 19:41

encapsulated props

ad34f70

encapsulated props but description is still not working

bc9e9cc

Enjoy2Live requested changes Apr 20, 2024

View reviewed changes

NAsriGhada and others added 4 commits May 4, 2024 20:58

new changes

0831545

use the same props coming from radix

2b1d5af

linked the progressbar element to its related text to improve accessi…

b09a08e

…blity see: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/progressbar_role#examples

install @radix-ui/react-progress

390902d

Enjoy2Live requested changes May 5, 2024

View reviewed changes

Merge pull request #1788 from dev-launchers/progressbar-changes

fc4570d

progressbar changes

Enjoy2Live merged commit 075dd76 into development/components May 6, 2024
7 of 9 checks passed

dev-launchers-flux added the released label Jun 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Components/progress bar #1756

Components/progress bar #1756

NAsriGhada commented Apr 2, 2024

changeset-bot bot commented Apr 2, 2024 •

edited

Loading

Enjoy2Live commented Apr 2, 2024

Enjoy2Live Apr 2, 2024

NAsriGhada Apr 2, 2024

Enjoy2Live left a comment

Enjoy2Live Apr 8, 2024

Enjoy2Live Apr 8, 2024

NAsriGhada Apr 9, 2024

Enjoy2Live Apr 9, 2024

Enjoy2Live Apr 8, 2024

Enjoy2Live Apr 8, 2024

Enjoy2Live Apr 8, 2024

Enjoy2Live Apr 8, 2024

Enjoy2Live Apr 8, 2024

Enjoy2Live Apr 8, 2024

Enjoy2Live Apr 8, 2024

Enjoy2Live Apr 17, 2024

NAsriGhada Apr 17, 2024

Enjoy2Live Apr 20, 2024

Enjoy2Live Apr 20, 2024

Enjoy2Live Apr 20, 2024

Enjoy2Live Apr 20, 2024

Enjoy2Live Apr 20, 2024 •

edited

Loading

Enjoy2Live left a comment

Enjoy2Live May 5, 2024

NAsriGhada May 6, 2024

dev-launchers-flux commented Jun 16, 2024

Components/progress bar #1756

Components/progress bar #1756

Conversation

NAsriGhada commented Apr 2, 2024

changeset-bot bot commented Apr 2, 2024 • edited Loading

⚠️ No Changeset found

Enjoy2Live commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enjoy2Live left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enjoy2Live Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

Enjoy2Live left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dev-launchers-flux commented Jun 16, 2024

changeset-bot bot commented Apr 2, 2024 •

edited

Loading

Enjoy2Live Apr 20, 2024 •

edited

Loading