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(Alert): add Alert component which uses @digdir/design-system-react #1289

Merged
merged 4 commits into from
Jul 31, 2023

Conversation

mikaelrss
Copy link
Contributor

@mikaelrss mikaelrss commented Jun 27, 2023

Description

Added a new Alert component, with options for

  • severity - sets the style of the alert, and has four valid values "success" | "info" | "danger" | "warning".
  • title - sets the title via textResourceBindings.
  • description - sets the description via textResourceBindings.
  • useAsAlert - A boolean which specifies whether the component should have role="alert" or not. It should not have this for static elements displaying information, but should have it for dynamic elements that appear after a certain user input. I currently implemented the aria-live attribute with polite, but I think maybe this should be configurable. Or maybe it should be based on severity label. if useAsAlert is set, and severity is >= warning, aria-live should be set to assertive. This option is removed and rather determined automatically based on the hidden property of GenericComponent. If hidden is explicitly set to false we append role="alert" to the Alert component. There is however one problem with this. If an element is hidden with an expression, but the expression evaluates to false on page load, role="alert" will be appended to the Alert component on load. The same problem would exist for the old API, where the app developers used a useAsAlert flag.

Screenshot:
image

Related Issue(s)

Verification/QA

  • Manual functionality testing
    • I have tested these changes manually
    • Creator of the original issue (or service owner) has been contacted for manual testing (or will be contacted when released in alpha)
    • No testing done/necessary
  • Automated tests
    • Unit test(s) have been added/updated
    • Cypress E2E test(s) have been added/updated - I have added some alerts to frontend-tests so that the alert is tested with percy. I also updated some failing cypress tests for Soft validations. Stopped using the custom id-based selector, and used @testing-library/cypress selectors for testing the content of alert messages instead.
    • No automatic tests are needed here (no functional changes/additions)
    • I want someone to help me make some tests
  • UU/WCAG (follow these guidelines until we have our own)
    • I have tested with a screen reader/keyboard navigation/automated wcag validator - Tested with wave and orca screen reader. The screen reader reads alerts twice for some reason, but I think this is an orca issue. Could somebody test with VoiceOver on Mac?
    • No testing done/necessary (no DOM/visual changes)
    • I want someone to help me perform accessibility testing
  • User documentation @ altinn-studio-docs
    • No functionality has been changed/added, so no documentation is needed
    • I will do that later/have created an issue
  • Changes/additions to component properties
    • Changes are reflected in both src/layout/layout.d.ts and layout.schema.v1.json, and these are all backwards-compatible
    • No changes made
  • Support in Altinn Studio
    • Issue(s) created for support in Studio
    • This change/feature does not require any changes to Altinn Studio
  • Sprint board
    • The original issue (or this PR itself) has been added to the Team Apps project and to the current sprint board
    • I don't have permissions to do that, please help me out
  • Labels
    • I have added a kind/* label to this PR for proper release notes grouping
    • I don't have permissions to add labels, please help me out

@mikaelrss mikaelrss added the kind/product-feature Pull requests containing new features label Jun 28, 2023
@mikaelrss mikaelrss force-pushed the feature/1284-alert-component branch from 369162f to 09b2fda Compare June 28, 2023 06:55
@mikaelrss
Copy link
Contributor Author

I see that I might need to update this PR and split out the schema for AlertComponent into a separate schema once #1283 is merged.

@mikaelrss mikaelrss marked this pull request as ready for review June 28, 2023 07:23
Copy link
Contributor

@Magnusrm Magnusrm left a comment

Choose a reason for hiding this comment

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

Nice work! 🏅 Just a thought about the useAsAlert property name 😁


export interface ILayoutCompAlertBase {
severity: AlertSeverity;
useAsAlert?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this property maybe needs a new name as the components name is Alert. 🤔 What about useAriaLive or something similar?

Side thought that might not work: Maybe it is possible to check if the Alert components required property in the layout file contains a boolean value or an expression(which is written as a string) and set this value to true if an expression is used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree! 💯 But I assume you mean the hidden property? An Alert is not a form component, so it cannot have any functionality for required.

Copy link
Contributor

Choose a reason for hiding this comment

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

@olemartinorg, Yes thats what i meant! Sorry 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that seems like a good idea. I'll see if that is possible 😅

@olemartinorg
Copy link
Contributor

This has a striking resemblance to the Panel component we already have, although this looks a bit different. Be sure to document what's the difference between these two components, and what's the use-case for each of them.

Also, this reminds me of #1023, which we closed because we wanted to introduce validations using expressions instead via #726. Does this Alert component have to be a component, or the use-case really just the same as soft validations? In that case, I think we should probably consider updating the visuals for the soft validations instead of making a new component.

@mikaelrss
Copy link
Contributor Author

This has a striking resemblance to the Panel component we already have, although this looks a bit different. Be sure to document what's the difference between these two components, and what's the use-case for each of them.

Also, this reminds me of #1023, which we closed because we wanted to introduce validations using expressions instead via #726. Does this Alert component have to be a component, or the use-case really just the same as soft validations? In that case, I think we should probably consider updating the visuals for the soft validations instead of making a new component.

Thanks for the feedback! Regarding #1023 @Febakke was involved in the startup meeting of this new component, and I think we said that this new component should take over the role of Panel in displaying messages which appear as some sort of user input. Please correct me if I am wrong here @Febakke

This component could also be used to display static messages which do not appear/disappear based on user input. Soft validations seem to me like they only appear as a validation message whenever the user provides some input.

Maybe we should have a chat about this component again @Febakke @olemartinorg @Magnusrm

@olemartinorg
Copy link
Contributor

I see! 🤔

This component could also be used to display static messages which do not appear/disappear based on user input.

The useAsAlert flag contradicts that. It opens up for using this as something that works just like a validation message, but a validation message that circumvents all the other (current and future) functionality in validation messages. I would assume there comes a feature request at some point for "having the option to not let the user proceed to the next page in the form when a certain Alert is displayed" and at that point we're on our way to re-implement validation with new clothes.

Soft validations seem to me like they only appear as a validation message whenever the user provides some input.

Ah, yes - so Alert is, in a sense, permanent soft validations. 🤔 Again, the functionality overlap here makes me feel uneasy. I don't want to be difficult, and I fully understand why you'd want to use a "fully featured component" for these messages, as it's much more flexible than regular validations (and permanent, as you mention!), but I think we should discuss this some more before letting it out in the wild. I would be very confused as an app developer if we promoted 3 different methods that achieve very similar things.

Maybe implementing an 'initial' trigger for validations could be an alternative (worryingly, validation messages disappear right now if you refresh the app, and you won't get them back until you hit a trigger), or reworking the way triggers on validations work could be another option.

Tagging @RonnyB71 as well.

@mikaelrss
Copy link
Contributor Author

mikaelrss commented Jun 28, 2023

The useAsAlert flag contradicts that. It opens up for using this as something that works just like a validation message, but a validation message that circumvents all the other (current and future) functionality in validation messages.

The thinking is that Alert can be used as both information and an alert that grabs the attention of a screen reader. This does not necessarily have to be a validation message, but can certainly be used as one also 😅. It could very well just be additional information that is displayed when making a user action.

For instance, in our specific use case in DGM we want to display this piece of extra information to the users when they select "Ja, jeg har informert de andre etterlatte", even though this is not a validation message.

image

Ah, yes - so Alert is, in a sense, permanent soft validations.

Yes, Alert is both soft validations and permanent soft validations, and just information 😅

In any case, it seems we need to discuss this component a little more in detail before we introduce it to the codebase.

@Febakke
Copy link
Member

Febakke commented Jun 28, 2023

@olemartinorg
I agree that this will be confusing for the app-developer and we have a lot of overlap here. I guess I did not think this trough when we had the initial meeting on this. 🐥 We should land on a plan on how to divide between alert and panel in a way that is easy to understand for our app-developers. Anyway, here are my thoughts on this. This is a suggestion on how it could work in an ideall world 😅

Alert
Alert is a defined pattern, and we use the same icons and severity grades as NAV does in their component. Our goal should be to use alert in the same way inn Apps. It should only contain text, preferbly short texts. It should not be used for validating a form. But in the future it might be used for giving the user information about a technical fault that is not related to what the user has filled in. "We could not submit your form because our database is full, sorry"

NB: I see that NAV has changed their documentation to say that you should not give the alert component: role=alert. Im not sure why, if the alert is presented after the page has been loaded it must have this role for screen readers to catch it. (They do say that it should not be presented dynamicly)

Summary:

  • Should be used for important short messages for the user
  • Should have different severity levels
  • Should not have buttons or other components inside them
  • Can be static or presented based on something that has happened either from the user or something outside of their control

Panel
When we first made our panel component, we had many different use cases and we thought it should handle all of them and we did at that time not think about concistensy across public solutions like NAV. This has been something that we have started to focus more on after starting to work on the design system.

The panel should be a grouping of elements that make it stand out from the rest of the page. It has no limitations on what it can contain. Example of uses, highlight parts of a paragraph you want to stand out or grouping form components with a text that is relevant.

The color should only be a stylistic choice, it should not inform the user of a grade of severity.

If it is presented to the user with dynamics it might need a live-region: polite depending on the content.

I have to 🏃, but there is also some overlap with soft validations that I have to think about in this context.

@mikaelrss
Copy link
Contributor Author

After a discussion in todays daily meeting, we have decided that we will introduce the new Alert component and at the same time replace the visual design of Soft validations to use the new Alert component from @digdir/design-system-react

@mikaelrss mikaelrss force-pushed the feature/1284-alert-component branch 8 times, most recently from 4c9dd43 to 625b5e1 Compare June 30, 2023 07:25
@sonarcloud
Copy link

sonarcloud bot commented Jun 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

91.8% 91.8% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@Magnusrm Magnusrm left a comment

Choose a reason for hiding this comment

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

Nice work! This looks good to me 🚀

@mikaelrss mikaelrss force-pushed the feature/1284-alert-component branch from 625b5e1 to 8bed79d Compare July 31, 2023 06:54
@sonarcloud
Copy link

sonarcloud bot commented Jul 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

93.3% 93.3% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@olemartinorg olemartinorg left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! 🚀

@mikaelrss mikaelrss merged commit 6554789 into main Jul 31, 2023
9 checks passed
@mikaelrss mikaelrss deleted the feature/1284-alert-component branch July 31, 2023 07:32
@mikaelrss mikaelrss self-assigned this Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/product-feature Pull requests containing new features
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Be able to use the Alert component from Felles Designsystem
4 participants