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

feat: add CellNumberfield to StudioInputTable #14345

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ErlingHauan
Copy link
Contributor

@ErlingHauan ErlingHauan commented Jan 2, 2025

Description

Adds CellNumberfield to StudioInputTable. It will be used later by StudioCodeListEditor for editing number values.

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

@github-actions github-actions bot added solution/studio/designer Issues related to the Altinn Studio Designer solution. frontend labels Jan 2, 2025
@ErlingHauan ErlingHauan changed the title Feat/cell decimal input feat: add CellNumberField to StudioInputTable Jan 6, 2025
@ErlingHauan ErlingHauan changed the title feat: add CellNumberField to StudioInputTable feat: add CellNumberfield to StudioInputTable Jan 6, 2025
@ErlingHauan ErlingHauan marked this pull request as ready for review January 6, 2025 09:34
@ErlingHauan ErlingHauan added the skip-manual-testing PRs that do not need to be tested manually label Jan 6, 2025
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.57%. Comparing base (465fc97) to head (7f5b8bd).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14345   +/-   ##
=======================================
  Coverage   95.57%   95.57%           
=======================================
  Files        1864     1865    +1     
  Lines       24151    24177   +26     
  Branches     2786     2788    +2     
=======================================
+ Hits        23082    23108   +26     
  Misses        812      812           
  Partials      257      257           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@standeren standeren self-assigned this Jan 7, 2025
Comment on lines 16 to 18
onChange?: (value: number) => void;
value?: number;
validationErrorMessage: string;
validationErrorMessage?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these optional? 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

onChange is made optional because I wanted to show it in the StudioInputTable storybook without attaching a handler function. Another option is to leave it as required and supplying it with an empty function. What do you think?

validationErrorMessage is made optional because it shows the error directly below the input field, which would look out of place in the table:
bilde

Copy link
Contributor

@standeren standeren Jan 7, 2025

Choose a reason for hiding this comment

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

Hm, in general I would think the code should be written with the components actual use and not be adapted to fit storybook or tests. So maybe an empty handler function in storybook would be preferable? 🤔

Also regarding the validationErrorMessage , I guess it would be better passing a undefined value for the storybook, or something similar?

The best solution could be to achieve a similar design as for the StudioCodeListEditor? But that would probably make more sense to implement in the PR that adopt the usage of CellNumberfield in the editor.
However, it might be a thing to keep in mind when implementing the error-handling for this component?
Skjermbilde 2025-01-07 kl  14 17 42

(Sorry for all the question marks here - I am not too sure about these allegations 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, in general I would think the code should be written with the components actual use and not be adapted to fit storybook or tests. So maybe an empty handler function in storybook would be preferable? 🤔

Agreed 😊

Also regarding the validationErrorMessage , I guess it would be better passing a undefined value for the storybook, or something similar?

But here it is not just for test or Storybook, but also how it is and will be implemented in StudioInputTable and StudioCodeListEditor. If there was a red validation message under each field, it would look out of place in the tables and would break the pattern we have in StudioCodeListEditor where the validation message is shown at the bottom.

The best solution could be to achieve a similar design as for the StudioCodeListEditor? But that would probably make more sense to implement in the PR that adopt the usage of CellNumberfield in the editor. However, it might be a thing to keep in mind when implementing the error-handling for this component? Skjermbilde 2025-01-07 kl 14 17 42

I agree with handling the validation message when implementing it in StudioCodeListEditor 😊

(Sorry for all the question marks here - I am not too sure about these allegations 😅)

It's OK 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand! Lets stick with the validationErrorMessage as optional 👏

@standeren standeren assigned ErlingHauan and unassigned standeren Jan 7, 2025
@ErlingHauan ErlingHauan assigned standeren and unassigned ErlingHauan Jan 7, 2025
@standeren standeren assigned ErlingHauan and unassigned standeren Jan 7, 2025
@ErlingHauan ErlingHauan assigned standeren and unassigned ErlingHauan Jan 8, 2025
@ErlingHauan ErlingHauan self-assigned this Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend skip-manual-testing PRs that do not need to be tested manually solution/studio/designer Issues related to the Altinn Studio Designer solution. team/studio-domain1
Projects
Status: 🔎 Review
Development

Successfully merging this pull request may close these issues.

2 participants