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

Implemented AppSelect component #268

Open
wants to merge 30 commits into
base: dev
Choose a base branch
from
Open

Conversation

RossWebFS
Copy link
Collaborator

No description provided.

@RossWebFS RossWebFS requested a review from mhevyk January 25, 2025 17:07
@RossWebFS RossWebFS self-assigned this Jan 25, 2025
@RossWebFS RossWebFS linked an issue Jan 25, 2025 that may be closed by this pull request
@RossWebFS RossWebFS marked this pull request as draft January 25, 2025 19:40
@RossWebFS

This comment was marked as resolved.

@RossWebFS

This comment was marked as resolved.

@RossWebFS RossWebFS marked this pull request as ready for review February 23, 2025 07:48
@RossWebFS RossWebFS requested a review from mhevyk February 23, 2025 07:48
@@ -27,7 +30,11 @@ const AppFormControl = forwardRef<HTMLDivElement, AppFormControlProps>(
}, [isInvalid, helperText]);

return (
<div className={classnames("app-form-control", className)} ref={ref}>
<div
style={{ width: fullWidth ? "100%" : "auto" }}
Copy link
Owner

Choose a reason for hiding this comment

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

Could you move it to scss file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,111 @@
@use "design-system/mixins/scrollbar.module.scss" as *;
Copy link
Owner

Choose a reason for hiding this comment

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

Let's use .module.scss only for exporting variables to js

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

type="multiple"
/>
<div style={{ marginTop: "20px" }}>
<h3>Selected Options:</h3>
Copy link
Owner

Choose a reason for hiding this comment

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

Same for texts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Already fixed

Comment on lines +116 to +183
<div ref={ref} style={{ width: containerWidth }}>
<div
style={{ width: containerWidth }}
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need to set width on both contaners?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed redundant style

};

// TODO: improve AppSpinner component
function AppSpinner({ variant }: AppSpinnerProps) {
function AppSpinner({ variant = "#000" }: AppSpinnerProps) {
Copy link
Owner

Choose a reason for hiding this comment

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

May I propose to use value form APP_COLORS?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines +3 to +5
$thumb-color: #ddd,
$thumb-hover: #ccc,
$track-color: #fff,
Copy link
Owner

Choose a reason for hiding this comment

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

Please use colors from design system

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines +8 to +24
&::-webkit-scrollbar {
width: $width; // for vertical scrollbar
height: $width; // for horizontal scrollbar
}

&::-webkit-scrollbar-track {
background: $track-color;
}

&::-webkit-scrollbar-thumb {
background: $thumb-color;
border-radius: $border-radius;
}

&::-webkit-scrollbar-thumb:hover {
background: $thumb-hover;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Did you check this in firefox? I think we need to add additional property because scrollbars are tricky to style across browsers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are not enough flexible styles for Firefox, so I can't fully customize the scrollbar

);
};

AppSelectItem.displayName = "Item";
Copy link
Owner

Choose a reason for hiding this comment

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

Please use AppSelectItem for display name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@mhevyk
Copy link
Owner

mhevyk commented Feb 23, 2025

image

Please add user-select: none to select button

@mhevyk
Copy link
Owner

mhevyk commented Feb 23, 2025

Please move all AppSelectItem related types, scss to separate folder within app-select, it should be like follows:

app-select/app-select-item

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.

Add AppSelect component
2 participants