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

frontend: Support for Workflow / Entire App Notification #3084

Merged
merged 7 commits into from
Sep 5, 2024

Conversation

lucechal14
Copy link
Contributor

@lucechal14 lucechal14 commented Aug 7, 2024

Description

Support for Notifications across the app. We have 3 types of notifications:

  1. Header: displayed in the header of the page with dynamic message, link and severity.
  2. Per Workflow: displayed at the top of the main section of the page for each workflow specified in the configuration of the notifications with dynamic title, message, link and severity per workflow.
  3. Multi Workflow: displayed at the top of the main section of the page for the workflows specified in the configuration of the notifications with dynamic title, message, link and severity.

Examples

  • Header and Per Workflow notification
Screenshot 2024-08-26 at 2 57 54 p m
  • Header and Multi Workflow notification
Screenshot 2024-08-26 at 2 59 53 p m

Testing

You can use an object similar to this to test:

const banners = { header: { message: "header message", linkText: "link", link: "linkhere", severity: "info", }, perWorkflow: { workflowName1: { title: "per workflow title for test 2", message: "per workflow message for test 2", linkText: "link", link: "linkhere", severity: "info", }, workflowName2: { title: "per workflow title for test 3", message: "per workflow message for test 3", linkText: "link", link: "linkhere", severity: "info", }, }, multiWorkflow: { title: "multi workflow title", message: "multi workflow message", workflows: ["workflowName1", "workflowName3"], severity: "info", linkText: "link", link: "linkhere", }, };

Send this const as banners in the appConfiguration prop of your ClutchApp

@lucechal14 lucechal14 requested a review from a team as a code owner August 7, 2024 15:57
@lucechal14 lucechal14 changed the title WIP frontend: feature - Adding Application Banner support frontend: Support for Workflow / Entire App Notification Aug 26, 2024
@jecr jecr removed the on hold label Aug 27, 2024
@lucechal14 lucechal14 requested a review from jecr September 3, 2024 16:57
@jecr
Copy link
Contributor

jecr commented Sep 3, 2024

LGTM, the only thing that feels odd is using the workflow's display name as key since it could be anything, didn't see an obvious alternative though 😅

@jecr
Copy link
Contributor

jecr commented Sep 3, 2024

Kind of an edge case for the header alert, a long link text can displace the description message. Also, long description messages don't take the available height and overflow at 2 lines.
I don't think the component should hold huge chunks of text like that, but some handling could come handy just in case.
image

@lucechal14
Copy link
Contributor Author

Kind of an edge case for the header alert, a long link text can displace the description message. Also, long description messages don't take the available height and overflow at 2 lines. I don't think the component should hold huge chunks of text like that, but some handling could come handy just in case. image

Yes the description has a set height and the it will overflow in case the description is too big, for the link the idea is that it is a small text, overall the idea of the header banner is just small text.

@jdslaugh
Copy link
Contributor

jdslaugh commented Sep 5, 2024

Kind of an edge case for the header alert, a long link text can displace the description message. Also, long description messages don't take the available height and overflow at 2 lines. I don't think the component should hold huge chunks of text like that, but some handling could come handy just in case. image

Yes the description has a set height and the it will overflow in case the description is too big, for the link the idea is that it is a small text, overall the idea of the header banner is just small text.

Good catch @jecr. @lucechal14 let's make sure it doesn't break free if it's too long. The idea is for it to not be very large but if it is I'd rather it be hidden or truncated versus breaking outside of the header. Can likely just overflow: scroll it and move the maxHeight to the alert itself.

@lucechal14
Copy link
Contributor Author

Kind of an edge case for the header alert, a long link text can displace the description message. Also, long description messages don't take the available height and overflow at 2 lines. I don't think the component should hold huge chunks of text like that, but some handling could come handy just in case. image

Yes the description has a set height and the it will overflow in case the description is too big, for the link the idea is that it is a small text, overall the idea of the header banner is just small text.

Good catch @jecr. @lucechal14 let's make sure it doesn't break free if it's too long. The idea is for it to not be very large but if it is I'd rather it be hidden or truncated versus breaking outside of the header. Can likely just overflow: scroll it and move the maxHeight to the alert itself.

@jdslaugh @jecr done, here is how it looks like
Screen Recording 2024-09-05 at 12 39 47 p m

@lucechal14 lucechal14 merged commit 008ef58 into main Sep 5, 2024
8 checks passed
@lucechal14 lucechal14 deleted the app-banners branch September 5, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants