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 StudioErrorMessage component #13943

Merged
merged 1 commit into from
Oct 31, 2024
Merged

Conversation

TomasEng
Copy link
Contributor

@TomasEng TomasEng commented Oct 29, 2024

Description

Added an StudioErrorMessage component based on the ErrorMessage component from The Design System.

image

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 Oct 29, 2024
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.13%. Comparing base (378bf66) to head (c7e5b04).
Report is 13 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #13943   +/-   ##
=======================================
  Coverage   95.13%   95.13%           
=======================================
  Files        1676     1678    +2     
  Lines       22232    22238    +6     
  Branches     2612     2612           
=======================================
+ Hits        21151    21157    +6     
  Misses        835      835           
  Partials      246      246           

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

@TomasEng TomasEng marked this pull request as ready for review October 29, 2024 14:12
@TomasEng TomasEng added skip-releasenotes Issues that do not make sense to list in our release notes skip-documentation Issues where updating documentation is not relevant team/studio-domain1 labels Oct 29, 2024
@standeren standeren self-assigned this Oct 31, 2024
Copy link
Contributor

@standeren standeren left a comment

Choose a reason for hiding this comment

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

Nice ⭐
I just have one question;
I know this is simply a wrapper around the ErrorMessage from DS, but I tested sending a StudioHeading to the children-prop in storybook and then it does not get the error style.
This is probably by design, but can it be a solution that we force string usage when the component is used in Studio in order to get the correct style? 🤔

@standeren standeren assigned TomasEng and unassigned standeren Oct 31, 2024
@TomasEng
Copy link
Contributor Author

Nice I just have one question; I know this is simply a wrapper around the ErrorMessage from DS, but I tested sending a StudioHeading to the children-prop in storybook and then it does not get the error style. This is probably by design, but can it be a solution that we force string usage when the component is used in Studio in order to get the correct style?

That might be a good idea, but I think it would be too strict. We may still want to use phrasing content inside the message. This is a typography component, so it is not supposed to contain block-level elements like headings.

@TomasEng TomasEng assigned standeren and unassigned TomasEng Oct 31, 2024
@standeren
Copy link
Contributor

Nice I just have one question; I know this is simply a wrapper around the ErrorMessage from DS, but I tested sending a StudioHeading to the children-prop in storybook and then it does not get the error style. This is probably by design, but can it be a solution that we force string usage when the component is used in Studio in order to get the correct style?

That might be a good idea, but I think it would be too strict. We may still want to use phrasing content inside the message. This is a typography component, so it is not supposed to contain block-level elements like headings.

Understand. Is it possible to restrict the children to be of those kind of elements? 🤔

@standeren standeren assigned TomasEng and unassigned standeren Oct 31, 2024
@TomasEng
Copy link
Contributor Author

Is it possible to restrict the children to be of those kind of elements?

Not with types. It could be possible with Javascript on runtime, but that' a quite unusual pattern that just adds unnecessary complexity to the component.

For some reason this component renders a <div> element, and not a <p> element. If it rendered a <p> element, we could use a tool like the W3C markup validator to validate this.

Anyways, I think it's best to keep the component API as close as possible to the one in The Design System until we come across a use case where it makes sense to narrow it down. We can always make a more specified component that extends this one.

@TomasEng TomasEng assigned standeren and unassigned TomasEng Oct 31, 2024
@standeren standeren removed their assignment Oct 31, 2024
@standeren standeren added the skip-manual-testing PRs that do not need to be tested manually label Oct 31, 2024
@standeren standeren merged commit fad4b91 into main Oct 31, 2024
14 checks passed
@standeren standeren deleted the error-message-component branch October 31, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend skip-documentation Issues where updating documentation is not relevant skip-manual-testing PRs that do not need to be tested manually skip-releasenotes Issues that do not make sense to list in our release notes solution/studio/designer Issues related to the Altinn Studio Designer solution. team/studio-domain1
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants