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

Make it possible to add action(s) to notifications #3999

Open
olensmar opened this issue May 26, 2023 · 11 comments
Open

Make it possible to add action(s) to notifications #3999

olensmar opened this issue May 26, 2023 · 11 comments
Assignees
Labels
enhancement New feature or request ux-ready The functionality is ready to be implemented

Comments

@olensmar
Copy link
Member

olensmar commented May 26, 2023

For certain notifications we might have a default action for the user - for example below we could show a user an action to "Update Dependencies" - would be easy to add as a callback..

image
@julioramosest
Copy link

Find UI design and notes here: https://www.figma.com/file/HOx9fsNedtkR2G0bCpHDX9/Resource-actions%2C-previewing-and-more?type=design&node-id=952%3A8873&t=gS0RAqPiep22C2ve-1

  • Icon changes (new one included in Ant (“Frown” filled)
  • Title color changes.
  • Save action goes text
  • color of actions right to the title goes link color (blue)
  • Main action goes bottom left and primary colored
  • Cancel action goes left to the primary one, secondary colored.
error modal rework

@julioramosest julioramosest removed their assignment May 29, 2023
@julioramosest julioramosest added ux-ready The functionality is ready to be implemented and removed needs-ux labels May 29, 2023
@olensmar
Copy link
Member Author

thanks @julioramosest - looks good!

@ayushgml
Copy link
Contributor

ayushgml commented Jun 6, 2023

@olensmar Can I be assigned this issue?
If i perceive this correctly then for styling I have to make changes in NotificationMarkdown.tsx and for default action we have to add CTA for each setAlert and to do that i have to add defaultAction in AlertType?

export enum DefaultAction {
   //...
};

type AlertType = {
  id?: string;
  title: string;
  message: string;
  type: AlertEnum;
  hasSeen?: boolean;
  createdAt?: number;
  duration?: number | null;
  extraContentType?: ExtraContentType;
  silent?: boolean;
  icon?: React.ReactNode;
  defaultAction: DefaultAction;      // Added this line
};

Please correct me if i missed something.

@devcatalin
Copy link
Member

Hi @ayushgml , we can assign you this issue and I have some useful information.

I've made a proof of concept for implementing these actions in the following draft PR: https://github.com/kubeshop/monokle/pull/4021/files

You can use that as a starting point for implementing actions.
The first action to implement could be the one mentioned by Ole, for updating dependencies of a Helm Chart.

Let us know if you need more help!

@ayushgml
Copy link
Contributor

ayushgml commented Jun 6, 2023

Thank you @devcatalin for assigning and for the information. I will use it to start working on it!

@ayushgml
Copy link
Contributor

ayushgml commented Jun 7, 2023

@devcatalin what types of actions can a button have? From the design i can infer 2 - Default action and cancel. Therefore I added a type:

type AlertButton = {
  type: 'cancel' | 'action'   <-----Added this
  text: string;
  action?: AnyAction;
  style?: React.CSSProperties;
};

Any more types it should be having?

@devcatalin
Copy link
Member

I don't think we need "cancel" explicitly since notifications already have an "X" button and they disappear after a few seconds.

I'd like the actions to be easily configured so that's what I tried if you look at the code changes from here:
https://github.com/kubeshop/monokle/pull/4021/files

I introduced a new AlertButton type:

type AlertButton = {
  text: string;
  action: AnyAction;
  style?: React.CSSProperties;
};

Where the text and style fields will be used for rendering and the action field is a Redux action that can be set for that button. When the button is rendered, it's onClick property will just dispatch that action, for example in that PR I added the following snippet in the NotificationMarkdown component:

<div>
  {buttons?.map(button => (
    <Button
      key={button.text}
      style={button.style}
      onClick={() => {
        dispatch(button.action);
      }}
    >
      {button.text}
    </Button>
  ))}
</div>

Then, there is an example of an alert with two buttons that are using two different redux actions:

{
  type: AlertEnum.Info,
  title: 'test',
  message: 'test',
  buttons: [
    {
      text: 'Sync action',
      action: testSyncAction(),
    },
    {
      text: 'Async action',
      action: triggerTestAsyncAction(),
    },
  ],
}

The only downside is that if we need to do some async logic from those buttons, which might be the case quite often, we have to dispatch a redux action and then use a redux listener to check if that action has been dispatched so that we can do an async effect.

Example:

export const testAsyncListener: AppListenerFn = listen => {
  listen({
    actionCreator: triggerTestAsyncAction,
    effect: async (action, {getState, dispatch}) => {
       console.log('doing some async action, with access to getState and dispatch');
    },
  });
};

I hope this helps! Let me know if you have more questions

@ayushgml
Copy link
Contributor

ayushgml commented Jun 7, 2023

Thank you @devcatalin for the reference. It helped me a lot!
I thought of adding type because if the the notification message is greater than 200 in length then there is a See more button. After clicking that a card like this appears.

Screenshot 2023-06-07 at 5 10 15 PM

This card doesn't goes away on it's own.

const handleSeeMore = () => {
    // @ts-ignore
    Modal[type]({
      content: (
        <S.NotificationModalContent>
          <ReactMarkdown
            components={{
              a({href, children, ...restProps}) {
                return (
                  <a onClick={() => openUrlInExternalBrowser(href)} {...restProps}>
                    {children}
                  </a>
                );
              },
            }}
          >
            {message}
          </ReactMarkdown>
        </S.NotificationModalContent>
      ),
      title: (
        <Provider store={store}>
          <NotificationModalTitle message={message} title={title} />
        </Provider>
      ),
      width: 600,
      cancelText: 'Cancel',   <------- This will close the card
      okText: 'Done', <--------- Default action text will be showed as here
    });
  };

Also this card is a Modal type from AntD design. The modal type has a fixed set of key value pairs and button types as here:

export interface ModalProps {
    .
    .
    .
    okText?: React.ReactNode;  <--------

    okType?: LegacyButtonType;

    cancelText?: React.ReactNode; <---------
   .
   .
   .
   .

I want to ask for the HandleShowMore type of card. @devcatalin

@devcatalin
Copy link
Member

We can render the custom buttons inside the content property of that modal and leaving the "cancel" and "done" buttons as they are now (just for closing that "see more" modal)

@ayushgml
Copy link
Contributor

ayushgml commented Jun 7, 2023

Thanks @devcatalin !

@ayushgml
Copy link
Contributor

ayushgml commented Jun 9, 2023

Hey @olensmar @devcatalin !! I implemened the thunk logic and the action trigger and tried it by triggering an error while previewing, it works. See below:
Screenshot 2023-06-09 at 2 14 04 PM

Now should I implement the same update Dependencies action for all Helm Errors or some particulars errors? And any other default actions except the Helm that I need to work on? I still have to style the buttons and card as present in the design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ux-ready The functionality is ready to be implemented
Projects
Status: No Status
Development

No branches or pull requests

4 participants