fix: global alert system implemented

[AronH99]

Description

• A global alert system has been implemented to work through a provider (AppAlertProvider) and a hook (useAppAlert). We now also only utilize 1 component (AppAlert.tsx) for all alerts now, both inline and toast, which can be reused throughout the whole console.

I tested most of the alerts manually throughout my process of replacing them with the new component.
Everything should work as before but now we have a system for it which makes it more maintainable :)
Went through all the automated tests, made adjustments wherever necessary and all of them passed for me.

EDIT: also a benefit now is that multiple toasts are supported now and they show up chronologically

#5933

[LukasStordeur]

I notice a few inconsistencies regarding the fallback empty string value for the message. Sometimes there's one and other times, there's none.

There's also a pattern notifyError(error.message, "", "error-toast-actions-error-message"); that is reccuring, where you need to pass an empty value to be able to pass in the aria-lable for the alert. This also obscures a bit what each value is at first glance.

I'd very much prefer an option object with set defaults where needed.
you'd get a notifyError({errorMessage: "message", errorAriaLabel: "aria-label-value", // etc...});
More readable, less brittle, and you only pass the values you need to pass.

[LukasStordeur]

I think you missed this part ;)

There's also a pattern notifyError(error.message, "", "error-toast-actions-error-message"); that is reccuring, where you need to pass an empty value to be able to pass in the aria-lable for the alert. This also obscures a bit what each value is at first glance.

I'd very much prefer an option object with set defaults where needed.
you'd get a notifyError({errorMessage: "message", errorAriaLabel: "aria-label-value", // etc...});
More readable, less brittle, and you only pass the values you need to pass.

[AronH99]

you are totally right, should have changed to to an object, is indeed more clear and less prune to issues.

EDIT: should be fixed now, good call :)

[LukasStordeur]

The logic is a bit over complicated, and can be improved/simplified. You don't need all those timers and to keep track of a counter.
You can use the typing already available in PF, and use Partial/Pick/Omit utils to get the pieces you need. Or extend if you need additional types.

These are limited snippets to give an idea of how you can refactor it.

import { v4 as uuidv4 } from "uuid"; // to get unique id/key
// rest of code

  const [alerts, setAlerts] = useState<Partial<AlertProps>[]>([]); // AlertProps are available in PF library.

 // strip the timers here, you don't need those. Each toaster has it's own timeout event you can hook into.
  const addAlert = (title: string, variant: AlertProps['variant'], key: React.Key) => {
    const uuid = uuidv4();
    setAlerts((prevAlerts) => [{ title, variant, key }, ...prevAlerts]);
  };

  const removeAlert = (key: React.Key) => {
    setAlerts((prevAlerts) => [...prevAlerts.filter((alert) => alert.key !== key)]);
  };

      <AlertGroup hasAnimations isToast isLiveRegion>
        {alerts.map(({ key, variant, title }) => (
          <Alert
            variant={AlertVariant[variant]}
            title={title}
            timeout="5000" // defaults to 8000
            onTimeout={() => removeAlert(key)}
            actionClose={
              <AlertActionCloseButton
                title={title as string}
                variantLabel={`${variant} alert`}
                onClose={() => removeAlert(key)}
              />
            }
            key={key}
          />
        ))}
      </AlertGroup>

[LukasStordeur]

On a sidenote, complementing the comment on the file on how to refactor, declaring it like this, will potentially leak accros modules. for example in tests environments, you will have contamination from one test to another if they remount a new provider each, which is often the case.

[AronH99]

You are right, didn't about this since it isn't really an issue right now but could be in the future yes.
I will look into a refactor because i didn't notice the timeout prop on the Alert itself but yes should make it less complex.

[LukasStordeur]

onclose ?? <AlertActionCloseButton onClose={onClose} data-testid={closeTestId} />
will result in the same. If you want to omit passing the property itself, that's also possible.
{...(onclose && { actionClose={<AlertActionCloseButton onClose={onClose} data-testid={closeTestId} />})}

[AronH99]

first one gives ts issues, second on is less readable imo

[LukasStordeur]

sorry, it's the && ipv ??

[LukasStordeur]

for all the props that don't need to be customized, you can use ...props and pass it directly to the jsx component.

[AronH99]

yes but i didn't do that in here because i'd like to limit the amount of props we can pass, preferably there will be no more extra props we can pass since it limits the devs to work with the existing ones and keep the design and implementation more consistent in general..
otherwise u can always extend your interface with the props of PF but that seems def overkill here

[LukasStordeur]

why? we are using the patternfly alert component. Propagating the props they support doesn't seem like more maintenance to me? On the contrary, declaring each prop explicitly looks like more maintenance.

[AronH99]

i adjusted it in favor of your suggestion

[LukasStordeur]

Still seeing all the standard properties being passed explicitly.
title, isInline, isExpandable, isPlain, customIcon, those can all be set with a ...rest or ...props on the Alert component.

[LukasStordeur]

pre-line might look better? I'm not sure the indentation is making it more readeable. And if there's only one line, the message would look a bit odd.

[AronH99]

i adjusted it to pre-line, pre-wrap was just something which was used most of the time in web-console but we can for sure change it and see if that works better.

[LukasStordeur]

I don't think there's much added value on making a differenciation here on "inline" for the message itself. You'd want the displayed text to respect newline characters present in the message we get from the api

[AronH99]

again this comes mostly from the way it was implemented in the app but we can try the StyledToastMessage as pre-line and see how it looks in general, i definitely give it a look in web-console
=> i wanted to make as few adjustments as possible to keep the look the same

[LukasStordeur]

It looks to me that message is required, I'd omit this conditional.

[AronH99]

message is sometimes not used so needs to be conditional atm

[AronH99]

All right addressed most comments properly ( going to see tomorrow morning if e2e passed then request review again )

EDIT: They passed :)

[LukasStordeur]

There were still open comments ;) Can you go over them again too?

[LukasStordeur]

The component can perfectly handle this internally, and add a derived test id on the close button if present. I know it requires updating some tests, but it's cleaner.

[LukasStordeur]

Why? Heading-order wasn't a problem previously, why is it now flagged and thus disabled?

[LukasStordeur]

So data attributes are a bit of a special thing. They are a built in thing in HTML and when you want to access native data attributes, you can always do that through the dataset property.
Having this in a custom interface like this can be a bit confusing.
I'd just go with the name testId and like mentionned in an onter comment, derive the close test id from this one internally.

If you want more info about the dataset attributes : https://developer.mozilla.org/en-US/docs/Web/HTML/How_to/Use_data_attributes

[LukasStordeur]

Still seeing all the standard properties being passed explicitly.
title, isInline, isExpandable, isPlain, customIcon, those can all be set with a ...rest or ...props on the Alert component.