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

fix(storybook): add functionality #1147

Merged
merged 6 commits into from
Aug 26, 2024

Conversation

bar-tay
Copy link
Contributor

@bar-tay bar-tay commented Jul 29, 2024

closes #1113

@bar-tay bar-tay linked an issue Jul 29, 2024 that may be closed by this pull request
4 tasks
@bar-tay bar-tay added the 🦹 needs:reviewers (code) The issue or pull request needs additional code reviewers label Jul 29, 2024
@thrbnhrtmnn thrbnhrtmnn self-requested a review July 29, 2024 14:17
Copy link
Contributor

@thrbnhrtmnn thrbnhrtmnn left a comment

Choose a reason for hiding this comment

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

Hey @bar-tay ,

from looking at storybook, you managed to complete all ACs, but you also broke some existing behaviour.

For me it seems like the width of the text area is now only controlled by the parent. The old option to define the width via the cols prop is gone. This should not have happened. We want both options, defining the width by the parent or defining the width via the cols. For this we probably need another property, similar to how the buttonDisplay prop in the Button Text component.

If you are decide for adding a new property, please place it under the "Appearance" category and also add a Description. While at it, please also add a short description to the "rows" property, which explains that we have a min height.

@thrbnhrtmnn thrbnhrtmnn removed the 🦹 needs:reviewers (code) The issue or pull request needs additional code reviewers label Aug 1, 2024
Copy link
Contributor

@thrbnhrtmnn thrbnhrtmnn left a comment

Choose a reason for hiding this comment

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

Hey @bar-tay ,
I see a few things that we need to look into:

  • If set to inline block, it seem like only the input field gets set to inline-block and the label container as well as the caption & counter container are still using the whole available width. This leads to the following unwanted behaviour:
    screenshot
    Instead the behaviour should be the same as on the current develop.

  • I see in the code, that you are using tokens for the MinHeight. I am not sure if they are the correct ones though, so please double check. In Figma it says to me the token for the LG viewport is called "Forms-TextArea-InputField-MinHeight-LG", in the code it looks to me you are using a token called "InputField.MinHeight.LG". Could you verify that you are using the correct token?

  • Only a small thing: Could you rename the "textareaDisplay" prop to "textAreaDisplay" and also place it below the "resize" and above the "cols" prop? Also please change the control form a select to a radio with both options.

  • Only a small thing: The "buttonDisplay" prop in the Button Text component seems to have been broken on your branch. There is no control and no description for the property.

  • Please have a look at the failing test and fix it as well.

@bar-tay
Copy link
Contributor Author

bar-tay commented Aug 23, 2024

Hey @bar-tay , I see a few things that we need to look into:

  • If set to inline block, it seem like only the input field gets set to inline-block and the label container as well as the caption & counter container are still using the whole available width. This leads to the following unwanted behaviour:
    screenshot
    Instead the behaviour should be the same as on the current develop.

For me, the state in develop also doesnt work. In my current state, I changed it so the label and the counter align with the textarea box when the size is changed. I also integrated that the appendix doesn't break when the label is super long. Maybe you have some input for that on further behaviour in the current state.

  • I see in the code, that you are using tokens for the MinHeight. I am not sure if they are the correct ones though, so please double check. In Figma it says to me the token for the LG viewport is called "Forms-TextArea-InputField-MinHeight-LG", in the code it looks to me you are using a token called "InputField.MinHeight.LG". Could you verify that you are using the correct token?

Yes, the correct values are used. I am destructuring the InputField.MinHeight... form the sem.TextArea in line 100 of the code. It is basically the same as when I would write TextArea.InputField.MinHeight... in a shorter form.

  • Only a small thing: Could you rename the "textareaDisplay" prop to "textAreaDisplay" and also place it below the "resize" and above the "cols" prop? Also please change the control form a select to a radio with both options.

Done

  • Only a small thing: The "buttonDisplay" prop in the Button Text component seems to have been broken on your branch. There is no control and no description for the property.

Done

  • Please have a look at the failing test and fix it as well.

Done

@thrbnhrtmnn thrbnhrtmnn merged commit d94e1a6 into develop Aug 26, 2024
4 of 5 checks passed
@thrbnhrtmnn thrbnhrtmnn deleted the fix/1113_adjust_size_of_input_field branch August 26, 2024 09:00
@thrbnhrtmnn
Copy link
Contributor

I already merged, as it looked good on my side as well and there was another approval. So this is my approval as well :-)

ashk1996 pushed a commit that referenced this pull request Aug 26, 2024
* fix(storybook): add functionality

* fix(storybook): textarea size limitations

* fix(storybook): add row describtion

* fix(storybook): review changes

---------

Co-authored-by: Thorben Hartmann <122102805+thrbnhrtmnn@users.noreply.github.com>
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.

Text Area - size of input field should be the same in design and code
3 participants