Conversation
GrabowskiM
requested review from
OstafinL,
RopRaptor,
albozek,
Copilot,
dew326 and
tischsoic
There was a problem hiding this comment.
Pull Request Overview
This PR establishes the foundation for inputs in the component library and brings various code consistency improvements.
- Introduces Form Label, Form Helper, Base Input, and Input Text components with corresponding tests
- Improves module aliasing, SCSS variable usage, and TypeScript type exports
- Updates Storybook configuration and dependency declarations to support new documentation features
Reviewed Changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/components/src/FormHelper/* | New FormHelper component, types, and stories for input helpers |
| packages/components/src/Expander/* | Updated alias for importing shared CSS class names |
| packages/components/src/Button/* | Adjusted Button prop ordering with added title attribute |
| packages/components/src/Accordion/* | Updated alias for TranslatorContext and shared CSS utilities |
| packages/components/.babelrc | Included module-resolver configuration for alias support |
| packages/assets/src/scss/* | Updated SCSS variables, transitions, and added new input/style files |
| package.json & .storybook/main.js | Added new dependencies and aliases to support Storybook addon-docs |
Comments suppressed due to low confidence (1)
packages/components/src/FormHelper/FormHelper.types.ts:9
- [nitpick] Consider renaming 'FormHelperTypesType' to 'FormHelperType' for clearer and more concise naming.
export type FormHelperTypesType = (typeof FORM_HELPER_TYPE_VALUES)[number];
| $transition-duration: 0.4s !default; | ||
|
|
||
| // Various component variables | ||
| $border-radius-medium: calculateRem(8px) !default; |
There was a problem hiding this comment.
We have font sizes with shortcut like -s or -m in name eg. $text-font-size-s, $text-font-size-m so maybe
| $border-radius-medium: calculateRem(8px) !default; | |
| $border-radius-size-m: calculateRem(8px) !default; |
| $admin-transition: cubic-bezier(0.25, 0.8, 0.25, 1) !default; | ||
| $admin-transition-duration: 0.4s !default; | ||
| $transition-timing-function: cubic-bezier(0.25, 0.8, 0.25, 1) !default; | ||
| $transition-duration: 0.4s !default; |
There was a problem hiding this comment.
What do you think about adding all animation times to variables? We have values 1s, 0.4s and 0.3s and only 0.4s is a variable
There was a problem hiding this comment.
In the future, Ida has to look into this topic what are recomendations and what transition times we need
| import { BaseComponentAriaAttributes } from '@ids-types/general'; | ||
|
|
||
| export const BUTTON_SIZE_VALUES = ['large', 'small', 'extra-small'] as const; | ||
| export const BUTTON_TYPE_VALUES = ['primary', 'secondary', 'tertiary', 'black-secondary', 'black-tertiary'] as const; |
There was a problem hiding this comment.
is it safe to use color names black-secondary, black-tertiary in button type?
There was a problem hiding this comment.
Ida addressed it in design, but it will be done in another task, you know... out of scope ;)
|
|
||
| export default meta; | ||
|
|
||
| type Story = StoryObj<typeof FormHelper>; |
There was a problem hiding this comment.
nitpick: I wonder if a name of this type is adequate. Maybe the name should carry a bit more information? Unfortunately I don't have a good idea for it, so it's just loud thinking :)
There was a problem hiding this comment.
Do you mean Story? Personally in this case I would give a pass, it's a little like convention for all stories for each component. But maybe I just get used to it by working with it from the start, so if anyone else would also suggest changing, I'll guess I'll bend to will of majority. ;)
Description:
Foundation for inputs:
Small improvements for code:
../../../importsFor QA:
Documentation: